diff --git a/.sampo/changesets/valiant-iceseeker-otso.md b/.sampo/changesets/valiant-iceseeker-otso.md new file mode 100644 index 000000000..5cb792019 --- /dev/null +++ b/.sampo/changesets/valiant-iceseeker-otso.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Reject negative capture retry counts diff --git a/posthog/capture_v1.py b/posthog/capture_v1.py index a4f199dde..4a52c60aa 100644 --- a/posthog/capture_v1.py +++ b/posthog/capture_v1.py @@ -491,8 +491,10 @@ def _send_v1_batch( unchanged. A transport failure re-raises the underlying exception (drops collected on an earlier attempt are still tallied in the DEBUG summary). ``request_id`` and the batch ``created_at`` are stable across attempts; - ``PostHog-Attempt`` increments. + ``PostHog-Attempt`` increments. Negative ``max_retries`` values are treated + as zero, so delivery is always attempted at least once. """ + max_retries = max(0, max_retries) request_id = str(uuid4()) # Hoisted once so the batch envelope is byte-identical across retry attempts # (only the events list shrinks and the attempt header increments). @@ -606,7 +608,6 @@ def _send_v1_batch( continue raise v1_error - # Unreachable in practice (every branch returns or continues), but keeps the - # function total if max_retries is somehow negative. + # Unreachable in practice (every branch returns or continues). if last_exc: raise last_exc diff --git a/posthog/client.py b/posthog/client.py index 9469407bd..398c61055 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -503,7 +503,7 @@ def __init__( flush_interval: Maximum seconds a background consumer waits before flushing a partial batch. gzip: Whether to gzip event upload payloads. - max_retries: Number of upload retries for background consumers. + max_retries: Number of upload retries. Values below 0 are treated as 0. sync_mode: If True, send each event synchronously instead of using background worker threads. timeout: HTTP request timeout in seconds for event uploads. @@ -607,7 +607,7 @@ def __init__( self._duplicate_client_registry_key: Optional[tuple[str, str]] = None self.gzip = gzip self.timeout = timeout - self.max_retries = max_retries + self.max_retries = max(0, max_retries) self._feature_flags: Optional[list[Any]] = ( None # private variable to store flags ) @@ -766,7 +766,7 @@ def __init__( flush_at=flush_at, flush_interval=flush_interval, gzip=gzip, - max_retries=max_retries, + max_retries=self.max_retries, timeout=timeout, historical_migration=historical_migration, ) diff --git a/posthog/consumer.py b/posthog/consumer.py index b60e156fb..3545973c1 100644 --- a/posthog/consumer.py +++ b/posthog/consumer.py @@ -77,7 +77,7 @@ def __init__( # run() *after* we set it to False in pause... and keep running # forever. self.running = True - self.retries = retries + self.retries = max(0, retries) self.timeout = timeout self.historical_migration = historical_migration diff --git a/posthog/test/test_capture_v1.py b/posthog/test/test_capture_v1.py index a368ae5d6..0846ae855 100644 --- a/posthog/test/test_capture_v1.py +++ b/posthog/test/test_capture_v1.py @@ -668,6 +668,17 @@ def test_transport_error_exhausted_reraises_original(self) -> None: ) self.assertEqual(len(stub.calls), 2) + def test_negative_max_retries_still_attempts_delivery_once(self) -> None: + stub = _PostV1Stub([_results_response({"u-1": "ok"})]) + + with mock.patch("posthog.capture_v1._post_v1", stub): + _send_v1_batch( + "phc_key", "https://app.posthog.com", [_msg("u-1")], max_retries=-1 + ) + + self.assertEqual(len(stub.calls), 1) + self.assertEqual(stub.calls[0]["attempt"], 1) + def test_small_retry_after_does_not_shorten_backoff(self) -> None: # A Retry-After smaller than the configured backoff must not make the # client retry earlier than its own schedule (Retry-After is a minimum). diff --git a/posthog/test/test_client.py b/posthog/test/test_client.py index fd798f7bc..eb9b8785d 100644 --- a/posthog/test/test_client.py +++ b/posthog/test/test_client.py @@ -3357,6 +3357,58 @@ def test_debug_flag_re_raises_exceptions(self, mock_enqueue): self.assertEqual(str(cm.exception), "Expected error") +class TestClientCaptureRetrySemantics(unittest.TestCase): + @parameterized.expand( + [ + ("v0_sync", "v0", True), + ("v1_sync", "v1", True), + ("v0_async", "v0", False), + ("v1_async", "v1", False), + ] + ) + def test_negative_max_retries_still_attempts_delivery_once( + self, _name, capture_mode, sync_mode + ): + response = mock.Mock(status_code=200, headers={}, text="") + response.json.return_value = {"results": {}} + client = None + + with ( + mock.patch("posthog.client.batch_post") as sync_v0_post, + mock.patch("posthog.consumer.batch_post") as async_v0_post, + mock.patch("posthog.capture_v1._post_v1", return_value=response) as v1_post, + ): + try: + client = Client( + FAKE_TEST_API_KEY, + capture_mode=capture_mode, + sync_mode=sync_mode, + max_retries=-1, + flush_at=1, + flush_interval=0.01, + ) + client.capture("evt", distinct_id="d") + if not sync_mode: + client.flush() + + self.assertEqual(client.max_retries, 0) + if capture_mode == "v1": + v1_post.assert_called_once() + sync_v0_post.assert_not_called() + async_v0_post.assert_not_called() + elif sync_mode: + sync_v0_post.assert_called_once() + async_v0_post.assert_not_called() + v1_post.assert_not_called() + else: + async_v0_post.assert_called_once() + sync_v0_post.assert_not_called() + v1_post.assert_not_called() + finally: + if client is not None and not sync_mode: + client.shutdown() + + class TestClientSyncCaptureMode(unittest.TestCase): """Sync-mode `_enqueue` selects the analytics submitter by `capture_mode`.""" diff --git a/posthog/test/test_consumer.py b/posthog/test/test_consumer.py index ab582193b..3affe9bae 100644 --- a/posthog/test/test_consumer.py +++ b/posthog/test/test_consumer.py @@ -149,6 +149,15 @@ def test_request_does_not_retry_client_errors(self) -> None: def test_request_fails_when_exceptions_exceed_retries(self) -> None: self._run_retry_test(APIError(500, "Internal Server Error"), 4, retries=3) + def test_negative_retries_still_attempts_delivery_once(self) -> None: + consumer = Consumer(None, TEST_API_KEY, retries=-1) + + with mock.patch("posthog.consumer.batch_post") as mock_post: + consumer.request([_track_event()]) + + self.assertEqual(consumer.retries, 0) + mock_post.assert_called_once() + def test_pause(self) -> None: consumer = Consumer(None, TEST_API_KEY) consumer.pause() diff --git a/references/public_api_snapshot.txt b/references/public_api_snapshot.txt index d83fbb742..0ef4da142 100644 --- a/references/public_api_snapshot.txt +++ b/references/public_api_snapshot.txt @@ -562,7 +562,7 @@ attribute posthog.client.Client.in_app_modules = in_app_modules attribute posthog.client.Client.is_server = is_server attribute posthog.client.Client.log = logging.getLogger('posthog') attribute posthog.client.Client.log_captured_exceptions = log_captured_exceptions -attribute posthog.client.Client.max_retries = max_retries +attribute posthog.client.Client.max_retries = max(0, max_retries) attribute posthog.client.Client.metrics: PostHogMetrics attribute posthog.client.Client.on_error = on_error attribute posthog.client.Client.personal_api_key = self.secret_key @@ -598,7 +598,7 @@ attribute posthog.consumer.Consumer.log = logging.getLogger('posthog') attribute posthog.consumer.Consumer.max_msg_size = max_msg_size attribute posthog.consumer.Consumer.on_error = on_error attribute posthog.consumer.Consumer.queue = queue -attribute posthog.consumer.Consumer.retries = retries +attribute posthog.consumer.Consumer.retries = max(0, retries) attribute posthog.consumer.Consumer.running = True attribute posthog.consumer.Consumer.timeout = timeout attribute posthog.consumer.MAX_MSG_SIZE = 900 * 1024