From 21a84605489dfcc7ec5526172edcea3c42627c25 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 11 Feb 2026 19:15:29 -0500 Subject: [PATCH 01/29] Improve HTTP response handling and retry logic - Add Authorization header with Basic auth (base64 encoded write key) - Add X-Retry-Count header on all requests (starts at 0) - Implement Retry-After header support (capped at 300s) - Retry-After attempts don't count against backoff retry budget - Add granular status code classification: - Retryable 4xx: 408, 410, 429, 460 - Non-retryable 4xx: 400, 401, 403, 404, 413, 422 - Retryable 5xx: all except 501, 505 - Non-retryable 5xx: 501, 505 - Replace backoff decorator with custom retry loop - Exponential backoff with jitter (0.5s base, 60s cap) - Clear OAuth token on 511 Network Authentication Required - 413 Payload Too Large is non-retryable - Add 30 new comprehensive tests (106 total tests) Aligns with analytics-java and analytics-next retry behavior. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/consumer.py | 140 +++++-- segment/analytics/request.py | 45 +- segment/analytics/test/test_consumer.py | 527 +++++++++++++++++++++++- segment/analytics/test/test_request.py | 153 ++++++- 4 files changed, 821 insertions(+), 44 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 157e3c93..2f939ac7 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -1,10 +1,10 @@ import logging import time +import random from threading import Thread -import backoff import json -from segment.analytics.request import post, APIError, DatetimeSerializer +from segment.analytics.request import post, APIError, DatetimeSerializer, parse_retry_after from queue import Empty @@ -120,40 +120,108 @@ def next(self): return items def request(self, batch): - """Attempt to upload the batch and retry before raising an error """ - - def fatal_exception(exc): - if isinstance(exc, APIError): - # retry on server errors and client errors - # with 429 status code (rate limited), - # don't retry on other client errors - return (400 <= exc.status < 500) and exc.status != 429 - elif isinstance(exc, FatalError): - return True - else: - # retry on all other errors (eg. network) - return False - - attempt_count = 0 - - @backoff.on_exception( - backoff.expo, - Exception, - max_tries=self.retries + 1, - giveup=fatal_exception, - on_backoff=lambda details: self.log.debug( - f"Retry attempt {details['tries']}/{self.retries + 1} after {details['elapsed']:.2f}s" - )) - def send_request(): - nonlocal attempt_count - attempt_count += 1 + """Attempt to upload the batch and retry before raising an error""" + + def is_retryable_status(status): + """ + Determine if a status code is retryable. + Retryable 4xx: 408, 410, 429, 460 + Non-retryable 4xx: 400, 401, 403, 404, 413, 422, and all other 4xx + Retryable 5xx: All except 501, 505 + Non-retryable 5xx: 501, 505 + """ + if 400 <= status < 500: + return status in (408, 410, 429, 460) + elif 500 <= status < 600: + return status not in (501, 505) + return False + + def should_use_retry_after(status): + """Check if status code should respect Retry-After header""" + return status in (408, 429, 503) + + total_attempts = 0 + backoff_attempts = 0 + max_backoff_attempts = self.retries + 1 + + while True: try: - return post(self.write_key, self.host, gzip=self.gzip, - timeout=self.timeout, batch=batch, proxies=self.proxies, - oauth_manager=self.oauth_manager) - except Exception as e: - if attempt_count >= self.retries + 1: - self.log.error(f"All {self.retries} retries exhausted. Final error: {e}") + # Make the request with current retry count + response = post( + self.write_key, + self.host, + gzip=self.gzip, + timeout=self.timeout, + batch=batch, + proxies=self.proxies, + oauth_manager=self.oauth_manager, + retry_count=total_attempts + ) + # Success + return response + + except FatalError as e: + # Non-retryable error + self.log.error(f"Fatal error after {total_attempts} attempts: {e}") raise - send_request() + except APIError as e: + total_attempts += 1 + + # Check if we should use Retry-After header + if should_use_retry_after(e.status) and e.response: + retry_after = parse_retry_after(e.response) + if retry_after: + self.log.debug( + f"Retry-After header present: waiting {retry_after}s (attempt {total_attempts})" + ) + time.sleep(retry_after) + continue # Does not count against backoff budget + + # Check if status is retryable + if not is_retryable_status(e.status): + self.log.error( + f"Non-retryable error {e.status} after {total_attempts} attempts: {e}" + ) + raise + + # Count this against backoff attempts + backoff_attempts += 1 + if backoff_attempts >= max_backoff_attempts: + self.log.error( + f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" + ) + raise + + # Calculate exponential backoff delay with jitter + base_delay = 0.5 * (2 ** (backoff_attempts - 1)) + jitter = random.uniform(0, 0.1 * base_delay) + delay = min(base_delay + jitter, 60) # Cap at 60 seconds + + self.log.debug( + f"Retry attempt {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " + f"after {delay:.2f}s for status {e.status}" + ) + time.sleep(delay) + + except Exception as e: + # Network errors or other exceptions - retry with backoff + total_attempts += 1 + backoff_attempts += 1 + + if backoff_attempts >= max_backoff_attempts: + self.log.error( + f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" + ) + raise + + # Calculate exponential backoff delay with jitter + base_delay = 0.5 * (2 ** (backoff_attempts - 1)) + jitter = random.uniform(0, 0.1 * base_delay) + delay = min(base_delay + jitter, 60) # Cap at 60 seconds + + self.log.debug( + f"Network error retry {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " + f"after {delay:.2f}s: {e}" + ) + time.sleep(delay) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index ab92b807..511a8a8b 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -3,6 +3,7 @@ from gzip import GzipFile import logging import json +import base64 from dateutil.tz import tzutc from requests.auth import HTTPBasicAuth from requests import sessions @@ -12,8 +13,31 @@ _session = sessions.Session() +# Maximum Retry-After delay to respect (5 minutes) +MAX_RETRY_AFTER_SECONDS = 300 -def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manager=None, **kwargs): + +def parse_retry_after(response): + """ + Parse Retry-After header from response. + Returns the delay in seconds, or None if header is not present or invalid. + Caps the value at MAX_RETRY_AFTER_SECONDS. + """ + retry_after = response.headers.get('Retry-After') + if not retry_after: + return None + + try: + # Try parsing as integer (delay in seconds) + delay = int(retry_after) + return min(delay, MAX_RETRY_AFTER_SECONDS) + except ValueError: + # Could be HTTP-date format, but for simplicity we'll skip that + # Most APIs use integer seconds + return None + + +def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manager=None, retry_count=0, **kwargs): """Post the `kwargs` to the API""" log = logging.getLogger('segment') body = kwargs @@ -28,10 +52,18 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag log.debug('making request: %s', data) headers = { 'Content-Type': 'application/json', - 'User-Agent': 'analytics-python/' + VERSION + 'User-Agent': 'analytics-python/' + VERSION, + 'X-Retry-Count': str(retry_count) } + + # Add Authorization header - prefer OAuth Bearer token, fallback to Basic auth if auth: headers['Authorization'] = 'Bearer {}'.format(auth) + else: + # Basic auth with write key (format: "writeKey:" encoded in base64) + credentials = '{}:'.format(write_key) + encoded = base64.b64encode(credentials.encode('utf-8')).decode('utf-8') + headers['Authorization'] = 'Basic {}'.format(encoded) if gzip: headers['Content-Encoding'] = 'gzip' @@ -60,24 +92,25 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag log.debug('data uploaded successfully') return res - if oauth_manager and res.status_code in [400, 401, 403]: + if oauth_manager and res.status_code in [400, 401, 403, 511]: oauth_manager.clear_token() try: payload = res.json() log.debug('received response: %s', payload) - raise APIError(res.status_code, payload['code'], payload['message']) + raise APIError(res.status_code, payload['code'], payload['message'], res) except ValueError: log.error('Unknown error: [%s] %s', res.status_code, res.reason) - raise APIError(res.status_code, 'unknown', res.text) + raise APIError(res.status_code, 'unknown', res.text, res) class APIError(Exception): - def __init__(self, status, code, message): + def __init__(self, status, code, message, response=None): self.message = message self.status = status self.code = code + self.response = response def __str__(self): msg = "[Segment] {0}: {1} ({2})" diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 83717266..1b9718be 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -8,7 +8,7 @@ except ImportError: from Queue import Queue -from segment.analytics.consumer import Consumer, MAX_MSG_SIZE +from segment.analytics.consumer import Consumer, MAX_MSG_SIZE, FatalError from segment.analytics.request import APIError @@ -220,3 +220,528 @@ def mock_post_fn(*args, **kwargs): args, kwargs = mock_post.call_args cls().assertIn('proxies', kwargs) cls().assertEqual(kwargs['proxies'], proxies) + + def test_retry_count_header_increments(self): + """Test that X-Retry-Count header increments on each retry""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + retry_counts = [] + + def mock_post_fn(*args, **kwargs): + retry_counts.append(kwargs.get('retry_count', 0)) + if len(retry_counts) < 3: + raise APIError(500, 'error', 'Server Error') + # Success on third attempt + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + consumer.request([track]) + + # Should have been called 3 times with retry counts 0, 1, 2 + self.assertEqual(retry_counts, [0, 1, 2]) + + def test_non_retryable_4xx_status_codes(self): + """Test that non-retryable 4xx errors are not retried""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + non_retryable_codes = [400, 401, 403, 404, 413, 422] + + for status_code in non_retryable_codes: + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise APIError(status_code, 'error', f'Client Error {status_code}') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + try: + consumer.request([track]) + except APIError as e: + self.assertEqual(e.status, status_code) + + # Should only be called once (no retries) + self.assertEqual(call_count, 1, f'Status {status_code} should not be retried') + + def test_retryable_4xx_status_codes(self): + """Test that retryable 4xx errors are retried""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + retryable_codes = [408, 410, 429, 460] + + for status_code in retryable_codes: + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + if call_count < 3: + raise APIError(status_code, 'error', f'Retryable Error {status_code}') + # Success on third attempt + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): # Mock sleep to speed up test + consumer.request([track]) + + # Should have been called 3 times + self.assertEqual(call_count, 3, f'Status {status_code} should be retried') + + def test_non_retryable_5xx_status_codes(self): + """Test that non-retryable 5xx errors are not retried""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + non_retryable_codes = [501, 505] + + for status_code in non_retryable_codes: + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise APIError(status_code, 'error', f'Server Error {status_code}') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + try: + consumer.request([track]) + except APIError as e: + self.assertEqual(e.status, status_code) + + # Should only be called once (no retries) + self.assertEqual(call_count, 1, f'Status {status_code} should not be retried') + + def test_retryable_5xx_status_codes(self): + """Test that retryable 5xx errors are retried""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + retryable_codes = [500, 502, 503, 504] + + for status_code in retryable_codes: + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + if call_count < 3: + raise APIError(status_code, 'error', f'Server Error {status_code}') + # Success on third attempt + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): # Mock sleep to speed up test + consumer.request([track]) + + # Should have been called 3 times + self.assertEqual(call_count, 3, f'Status {status_code} should be retried') + + def test_retry_after_header_support(self): + """Test that Retry-After header is respected and doesn't count against retry budget""" + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + sleep_durations = [] + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + + if call_count <= 3: + # Return 429 with Retry-After for first 3 attempts + response = mock.Mock() + response.headers = {'Retry-After': '10'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + + # Success on 4th attempt + return mock.Mock(status_code=200) + + def mock_sleep(duration): + sleep_durations.append(duration) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + # Should succeed after 4 attempts (3 Retry-After, then success) + self.assertEqual(call_count, 4) + + # First 3 sleeps should be for Retry-After (10 seconds each) + self.assertEqual(sleep_durations[:3], [10, 10, 10]) + + def test_retry_after_capped_at_300_seconds(self): + """Test that Retry-After delay is capped at 300 seconds""" + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + sleep_duration = None + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + + if call_count == 1: + # Return 429 with large Retry-After + response = mock.Mock() + response.headers = {'Retry-After': '600'} # 10 minutes + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + + # Success on 2nd attempt + return mock.Mock(status_code=200) + + def mock_sleep(duration): + nonlocal sleep_duration + sleep_duration = duration + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + # Sleep should be capped at 300 seconds + self.assertEqual(sleep_duration, 300) + + def test_retry_after_for_408_and_503(self): + """Test that Retry-After is respected for 408 and 503 status codes""" + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + for status_code in [408, 503]: + call_count = 0 + sleep_duration = None + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + + if call_count == 1: + response = mock.Mock() + response.headers = {'Retry-After': '5'} + error = APIError(status_code, 'error', 'Error') + error.response = response + raise error + + return mock.Mock(status_code=200) + + def mock_sleep(duration): + nonlocal sleep_duration + sleep_duration = duration + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + self.assertEqual(sleep_duration, 5, f'Retry-After should be respected for {status_code}') + + def test_exponential_backoff_with_jitter(self): + """Test that exponential backoff is used for retries without Retry-After""" + consumer = Consumer(None, 'testsecret', retries=4) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + sleep_durations = [] + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + + if call_count <= 3: + raise APIError(500, 'error', 'Server Error') + + return mock.Mock(status_code=200) + + def mock_sleep(duration): + sleep_durations.append(duration) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + # Should have 3 backoff delays + self.assertEqual(len(sleep_durations), 3) + + # Delays should be increasing (exponential) + # First: ~0.5s, Second: ~1s, Third: ~2s (with jitter) + self.assertGreater(sleep_durations[0], 0.4) + self.assertLess(sleep_durations[0], 1.0) + self.assertGreater(sleep_durations[1], 0.9) + self.assertLess(sleep_durations[1], 2.0) + self.assertGreater(sleep_durations[2], 1.8) + self.assertLess(sleep_durations[2], 4.0) + + def test_fatal_error_not_retried(self): + """Test that FatalError is not retried""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise FatalError('Fatal error occurred') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + try: + consumer.request([track]) + except FatalError: + pass + + # Should only be called once (no retries) + self.assertEqual(call_count, 1) + + def test_max_retries_exhausted(self): + """Test that request fails after max retries exhausted""" + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + # Always fail with retryable error + raise APIError(500, 'error', 'Server Error') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): # Mock sleep to speed up test + try: + consumer.request([track]) + except APIError as e: + self.assertEqual(e.status, 500) + + # Should be called 3 times (initial + 2 retries) + self.assertEqual(call_count, 3) + + def test_first_request_has_retry_count_zero(self): + """T01: First successful request includes X-Retry-Count=0""" + consumer = Consumer(None, 'testsecret') + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + retry_count = None + + def mock_post_fn(*args, **kwargs): + nonlocal retry_count + retry_count = kwargs.get('retry_count') + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + consumer.request([track]) + + # First request should have retry_count=0 + self.assertEqual(retry_count, 0) + + def test_429_without_retry_after_uses_backoff(self): + """T09: 429 without Retry-After header uses backoff retry""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + retry_counts = [] + sleep_duration = None + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + retry_counts.append(kwargs.get('retry_count', 0)) + + if call_count == 1: + # 429 without Retry-After header + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = mock.Mock() + error.response.headers = {} # No Retry-After + raise error + + return mock.Mock(status_code=200) + + def mock_sleep(duration): + nonlocal sleep_duration + sleep_duration = duration + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + # Should have two attempts + self.assertEqual(call_count, 2) + self.assertEqual(retry_counts, [0, 1]) + + # Should use backoff delay (around 0.5s with jitter) + self.assertIsNotNone(sleep_duration) + if sleep_duration is not None: + self.assertGreater(sleep_duration, 0.4) + self.assertLess(sleep_duration, 1.0) + + def test_408_without_retry_after_uses_backoff(self): + """T10: 408 without Retry-After header uses backoff retry""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + retry_counts = [] + sleep_duration = None + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + retry_counts.append(kwargs.get('retry_count', 0)) + + if call_count == 1: + # 408 without Retry-After header + error = APIError(408, 'timeout', 'Request Timeout') + error.response = mock.Mock() + error.response.headers = {} # No Retry-After + raise error + + return mock.Mock(status_code=200) + + def mock_sleep(duration): + nonlocal sleep_duration + sleep_duration = duration + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + # Should have two attempts + self.assertEqual(call_count, 2) + self.assertEqual(retry_counts, [0, 1]) + + # Should use backoff delay + self.assertIsNotNone(sleep_duration) + if sleep_duration is not None: + self.assertGreater(sleep_duration, 0.4) + self.assertLess(sleep_duration, 1.0) + + def test_network_error_retried_with_backoff(self): + """T15: Network/IO error is retried with backoff""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + retry_counts = [] + sleep_duration = None + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + retry_counts.append(kwargs.get('retry_count', 0)) + + if call_count == 1: + # Network error + raise ConnectionError('Network connection failed') + + return mock.Mock(status_code=200) + + def mock_sleep(duration): + nonlocal sleep_duration + sleep_duration = duration + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep', side_effect=mock_sleep): + consumer.request([track]) + + # Should have two attempts + self.assertEqual(call_count, 2) + self.assertEqual(retry_counts, [0, 1]) + + # Should use backoff delay + self.assertIsNotNone(sleep_duration) + if sleep_duration is not None: + self.assertGreater(sleep_duration, 0.4) + self.assertLess(sleep_duration, 1.0) + + def test_511_is_retryable(self): + """T05: 511 status code is retryable (part of 5xx family, not in non-retryable list)""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + retry_counts = [] + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + retry_counts.append(kwargs.get('retry_count', 0)) + + if call_count < 3: + raise APIError(511, 'auth_required', 'Network Authentication Required') + + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): # Mock sleep to speed up test + consumer.request([track]) + + # Should have been called 3 times (511 is retryable) + self.assertEqual(call_count, 3) + self.assertEqual(retry_counts, [0, 1, 2]) + + def test_retry_after_not_counted_against_backoff_budget(self): + """T17: Retry-After attempts don't consume backoff retry budget""" + consumer = Consumer(None, 'testsecret', retries=1) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + retry_counts = [] + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + retry_counts.append(kwargs.get('retry_count', 0)) + + if call_count <= 2: + # First two: 429 with Retry-After (shouldn't count against budget) + response = mock.Mock() + response.headers = {'Retry-After': '1'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + elif call_count == 3: + # Third: 500 without Retry-After (counts against budget) + raise APIError(500, 'error', 'Server Error') + + # Success on 4th attempt + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): # Mock sleep to speed up test + consumer.request([track]) + + # Should succeed after 4 attempts: + # - 2 Retry-After attempts (don't count against budget) + # - 1 backoff attempt (counts against budget = 1) + # - 1 final backoff attempt (counts against budget = 1, limit reached) + # Actually wait, with retries=1, we have max_backoff_attempts=2 + # So: 2 Retry-After + 2 backoff attempts = 4 total + self.assertEqual(call_count, 4) + self.assertEqual(retry_counts, [0, 1, 2, 3]) + + def test_413_payload_too_large_not_retried(self): + """T12: 413 Payload Too Large is non-retryable (won't succeed on retry)""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise APIError(413, 'payload_too_large', 'Payload Too Large') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + try: + consumer.request([track]) + except APIError as e: + self.assertEqual(e.status, 413) + + # Should only be called once (no retries) + self.assertEqual(call_count, 1) diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 5ffca009..54b48bea 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -2,9 +2,10 @@ import unittest import json import requests +import base64 from unittest import mock -from segment.analytics.request import post, DatetimeSerializer +from segment.analytics.request import post, DatetimeSerializer, parse_retry_after, APIError class TestRequests(unittest.TestCase): @@ -72,3 +73,153 @@ def mock_post_fn(*args, **kwargs): args, kwargs = mock_post.call_args self.assertIn('proxies', kwargs) self.assertEqual(kwargs['proxies'], proxies) + + def test_authorization_header_basic_auth(self): + """Test that Basic Authorization header is added when no OAuth manager""" + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = 200 + return res + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn) as mock_post: + post('testsecret', batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + + args, kwargs = mock_post.call_args + headers = kwargs['headers'] + self.assertIn('Authorization', headers) + + # Verify it's Basic auth with correct encoding + expected_credentials = base64.b64encode(b'testsecret:').decode('utf-8') + expected_auth = f'Basic {expected_credentials}' + self.assertEqual(headers['Authorization'], expected_auth) + + def test_authorization_header_oauth(self): + """Test that Bearer Authorization header is used with OAuth manager""" + oauth_manager = mock.Mock() + oauth_manager.get_token.return_value = 'test_token_123' + + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = 200 + return res + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn) as mock_post: + post('testsecret', oauth_manager=oauth_manager, batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + + args, kwargs = mock_post.call_args + headers = kwargs['headers'] + self.assertIn('Authorization', headers) + self.assertEqual(headers['Authorization'], 'Bearer test_token_123') + + def test_x_retry_count_header(self): + """Test that X-Retry-Count header is included""" + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = 200 + return res + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn) as mock_post: + # Test with retry_count=0 (first attempt) + post('testsecret', retry_count=0, batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + + args, kwargs = mock_post.call_args + headers = kwargs['headers'] + self.assertIn('X-Retry-Count', headers) + self.assertEqual(headers['X-Retry-Count'], '0') + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn) as mock_post: + # Test with retry_count=5 + post('testsecret', retry_count=5, batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + + args, kwargs = mock_post.call_args + headers = kwargs['headers'] + self.assertEqual(headers['X-Retry-Count'], '5') + + def test_parse_retry_after_integer(self): + """Test parsing Retry-After header with integer seconds""" + response = mock.Mock() + response.headers = {'Retry-After': '30'} + result = parse_retry_after(response) + self.assertEqual(result, 30) + + def test_parse_retry_after_capped(self): + """Test that Retry-After is capped at 300 seconds""" + response = mock.Mock() + response.headers = {'Retry-After': '600'} + result = parse_retry_after(response) + self.assertEqual(result, 300) + + def test_parse_retry_after_missing(self): + """Test parsing when Retry-After header is missing""" + response = mock.Mock() + response.headers = {} + result = parse_retry_after(response) + self.assertIsNone(result) + + def test_parse_retry_after_invalid(self): + """Test parsing with invalid Retry-After header""" + response = mock.Mock() + response.headers = {'Retry-After': 'invalid'} + result = parse_retry_after(response) + self.assertIsNone(result) + + def test_oauth_token_cleared_on_511(self): + """Test that OAuth token is cleared on 511 status""" + oauth_manager = mock.Mock() + oauth_manager.get_token.return_value = 'test_token' + + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = 511 + res.json.return_value = {'code': 'error', 'message': 'Network Authentication Required'} + return res + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn): + try: + post('testsecret', oauth_manager=oauth_manager, batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + except APIError: + pass + + # Verify clear_token was called + oauth_manager.clear_token.assert_called_once() + + def test_api_error_includes_response(self): + """Test that APIError includes the response object""" + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = 429 + res.json.return_value = {'code': 'rate_limit', 'message': 'Too Many Requests'} + return res + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn): + try: + post('testsecret', batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + except APIError as e: + self.assertEqual(e.status, 429) + self.assertIsNotNone(e.response) + else: + self.fail('Expected APIError to be raised') From e4f34d0d377bb608b42537f31aeb061f6ec5d5b5 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 11 Feb 2026 20:06:09 -0500 Subject: [PATCH 02/29] Increase max retries from 10 to 1000 Aligns with analytics-java change to accommodate shorter backoff periods (0.5s base, 60s cap). With faster retries, a higher retry limit allows for better resilience during extended outages. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/client.py | 2 +- segment/analytics/consumer.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/segment/analytics/client.py b/segment/analytics/client.py index 0f8015cd..79262e90 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -30,7 +30,7 @@ class DefaultConfig(object): max_queue_size = 10000 gzip = False timeout = 15 - max_retries = 10 + max_retries = 1000 proxies = None thread = 1 upload_interval = 0.5 diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 2f939ac7..db0fe9d1 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -29,7 +29,7 @@ class Consumer(Thread): log = logging.getLogger('segment') def __init__(self, queue, write_key, upload_size=100, host=None, - on_error=None, upload_interval=0.5, gzip=False, retries=10, + on_error=None, upload_interval=0.5, gzip=False, retries=1000, timeout=15, proxies=None, oauth_manager=None): """Create a consumer thread.""" Thread.__init__(self) From 8bd4163e6d470454545b90cc782bba8902b806c2 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 12 Feb 2026 18:07:48 -0500 Subject: [PATCH 03/29] Update segment/analytics/test/test_request.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- segment/analytics/test/test_request.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 54b48bea..e0206dda 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -191,14 +191,12 @@ def mock_post_fn(*args, **kwargs): return res with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn): - try: + with self.assertRaises(APIError): post('testsecret', oauth_manager=oauth_manager, batch=[{ 'userId': 'userId', 'event': 'python event', 'type': 'track' }]) - except APIError: - pass # Verify clear_token was called oauth_manager.clear_token.assert_called_once() From e1ac2143dff29adcba581564f758c284038568ea Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 18 Feb 2026 10:46:10 -0500 Subject: [PATCH 04/29] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- segment/analytics/request.py | 5 +++-- segment/analytics/test/test_consumer.py | 4 +--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 511a8a8b..38a1be99 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -5,7 +5,7 @@ import json import base64 from dateutil.tz import tzutc -from requests.auth import HTTPBasicAuth + from requests import sessions from segment.analytics.version import VERSION @@ -30,7 +30,8 @@ def parse_retry_after(response): try: # Try parsing as integer (delay in seconds) delay = int(retry_after) - return min(delay, MAX_RETRY_AFTER_SECONDS) + # Ensure delay is non-negative before applying upper bound + return min(max(delay, 0), MAX_RETRY_AFTER_SECONDS) except ValueError: # Could be HTTP-date format, but for simplicity we'll skip that # Most APIs use integer seconds diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 1b9718be..3e8a8eb1 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -490,10 +490,8 @@ def mock_post_fn(*args, **kwargs): raise FatalError('Fatal error occurred') with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - try: + with self.assertRaises(FatalError): consumer.request([track]) - except FatalError: - pass # Should only be called once (no retries) self.assertEqual(call_count, 1) From 6a10b7c314451cfadb216cb31454d3f4aab264ab Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 18 Feb 2026 21:36:20 -0500 Subject: [PATCH 05/29] Address PR review feedback - Extract duplicate exponential backoff calculation into helper function - Add upper bound (max_total_attempts) to prevent infinite retry loops with Retry-After - Improves code maintainability and prevents edge case of continuous Retry-After responses Co-Authored-By: Claude Opus 4.6 --- segment/analytics/consumer.py | 35 ++++++++++++++++++++----- segment/analytics/test/test_consumer.py | 28 +++++++++----------- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index db0fe9d1..1d822017 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -140,9 +140,22 @@ def should_use_retry_after(status): """Check if status code should respect Retry-After header""" return status in (408, 429, 503) + def calculate_backoff_delay(attempt): + """ + Calculate exponential backoff delay with jitter. + First retry is immediate, then 0.5s, 1s, 2s, 4s, etc. + """ + if attempt == 1: + return 0 # First retry is immediate + base_delay = 0.5 * (2 ** (attempt - 2)) + jitter = random.uniform(0, 0.1 * base_delay) + return min(base_delay + jitter, 60) # Cap at 60 seconds + total_attempts = 0 backoff_attempts = 0 max_backoff_attempts = self.retries + 1 + # Prevent infinite retry loops even with Retry-After + max_total_attempts = max_backoff_attempts * 10 while True: try: @@ -168,6 +181,13 @@ def should_use_retry_after(status): except APIError as e: total_attempts += 1 + # Prevent infinite retry loops + if total_attempts >= max_total_attempts: + self.log.error( + f"Maximum total attempts ({max_total_attempts}) reached after {total_attempts} attempts. Final error: {e}" + ) + raise + # Check if we should use Retry-After header if should_use_retry_after(e.status) and e.response: retry_after = parse_retry_after(e.response) @@ -194,9 +214,7 @@ def should_use_retry_after(status): raise # Calculate exponential backoff delay with jitter - base_delay = 0.5 * (2 ** (backoff_attempts - 1)) - jitter = random.uniform(0, 0.1 * base_delay) - delay = min(base_delay + jitter, 60) # Cap at 60 seconds + delay = calculate_backoff_delay(backoff_attempts) self.log.debug( f"Retry attempt {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " @@ -209,6 +227,13 @@ def should_use_retry_after(status): total_attempts += 1 backoff_attempts += 1 + # Prevent infinite retry loops + if total_attempts >= max_total_attempts: + self.log.error( + f"Maximum total attempts ({max_total_attempts}) reached after {total_attempts} attempts. Final error: {e}" + ) + raise + if backoff_attempts >= max_backoff_attempts: self.log.error( f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" @@ -216,9 +241,7 @@ def should_use_retry_after(status): raise # Calculate exponential backoff delay with jitter - base_delay = 0.5 * (2 ** (backoff_attempts - 1)) - jitter = random.uniform(0, 0.1 * base_delay) - delay = min(base_delay + jitter, 60) # Cap at 60 seconds + delay = calculate_backoff_delay(backoff_attempts) self.log.debug( f"Network error retry {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 3e8a8eb1..df4b5314 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -469,13 +469,12 @@ def mock_sleep(duration): self.assertEqual(len(sleep_durations), 3) # Delays should be increasing (exponential) - # First: ~0.5s, Second: ~1s, Third: ~2s (with jitter) - self.assertGreater(sleep_durations[0], 0.4) - self.assertLess(sleep_durations[0], 1.0) - self.assertGreater(sleep_durations[1], 0.9) - self.assertLess(sleep_durations[1], 2.0) - self.assertGreater(sleep_durations[2], 1.8) - self.assertLess(sleep_durations[2], 4.0) + # First: 0s (immediate), Second: ~0.5s, Third: ~1s (with jitter) + self.assertEqual(sleep_durations[0], 0) # First retry is immediate + self.assertGreater(sleep_durations[1], 0.4) + self.assertLess(sleep_durations[1], 0.6) + self.assertGreater(sleep_durations[2], 0.9) + self.assertLess(sleep_durations[2], 1.2) def test_fatal_error_not_retried(self): """Test that FatalError is not retried""" @@ -572,11 +571,10 @@ def mock_sleep(duration): self.assertEqual(call_count, 2) self.assertEqual(retry_counts, [0, 1]) - # Should use backoff delay (around 0.5s with jitter) + # First retry should be immediate (0s delay) self.assertIsNotNone(sleep_duration) if sleep_duration is not None: - self.assertGreater(sleep_duration, 0.4) - self.assertLess(sleep_duration, 1.0) + self.assertEqual(sleep_duration, 0) def test_408_without_retry_after_uses_backoff(self): """T10: 408 without Retry-After header uses backoff retry""" @@ -613,11 +611,10 @@ def mock_sleep(duration): self.assertEqual(call_count, 2) self.assertEqual(retry_counts, [0, 1]) - # Should use backoff delay + # First retry should be immediate (0s delay) self.assertIsNotNone(sleep_duration) if sleep_duration is not None: - self.assertGreater(sleep_duration, 0.4) - self.assertLess(sleep_duration, 1.0) + self.assertEqual(sleep_duration, 0) def test_network_error_retried_with_backoff(self): """T15: Network/IO error is retried with backoff""" @@ -651,11 +648,10 @@ def mock_sleep(duration): self.assertEqual(call_count, 2) self.assertEqual(retry_counts, [0, 1]) - # Should use backoff delay + # First retry should be immediate (0s delay) self.assertIsNotNone(sleep_duration) if sleep_duration is not None: - self.assertGreater(sleep_duration, 0.4) - self.assertLess(sleep_duration, 1.0) + self.assertEqual(sleep_duration, 0) def test_511_is_retryable(self): """T05: 511 status code is retryable (part of 5xx family, not in non-retryable list)""" From 6ed8ef6a005d23590c0908fa28c933c5762cddd4 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 25 Feb 2026 11:16:33 -0500 Subject: [PATCH 06/29] Implement unified HTTP response handling per SDD - Remove 408/503 from Retry-After eligibility (only 429 uses Retry-After) - Add rate-limit state to Consumer (rate_limited_until, rate_limit_start_time) - 429 with Retry-After: set rate-limit state, raise to caller for requeue - 429 without Retry-After: counted backoff (not pipeline blocking) - Add maxTotalBackoffDuration / maxRateLimitDuration config (default 43200s) - upload() checks rate-limit state before request(), enforces duration limit - 511 OAuth gating: only retry when OauthManager is configured - Add tests: T04, T17, T19, T20; update 429/408/503 behavior tests Co-Authored-By: Claude Opus 4.6 --- segment/analytics/client.py | 15 +- segment/analytics/consumer.py | 145 ++++++++--- segment/analytics/test/test_consumer.py | 328 +++++++++++++++--------- 3 files changed, 332 insertions(+), 156 deletions(-) diff --git a/segment/analytics/client.py b/segment/analytics/client.py index 79262e90..5cb09734 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -31,6 +31,8 @@ class DefaultConfig(object): gzip = False timeout = 15 max_retries = 1000 + max_total_backoff_duration = 43200 + max_rate_limit_duration = 43200 proxies = None thread = 1 upload_interval = 0.5 @@ -65,9 +67,16 @@ def __init__(self, oauth_client_key=DefaultConfig.oauth_client_key, oauth_key_id=DefaultConfig.oauth_key_id, oauth_auth_server=DefaultConfig.oauth_auth_server, - oauth_scope=DefaultConfig.oauth_scope,): + oauth_scope=DefaultConfig.oauth_scope, + max_total_backoff_duration=DefaultConfig.max_total_backoff_duration, + max_rate_limit_duration=DefaultConfig.max_rate_limit_duration,): require('write_key', write_key, str) + if max_total_backoff_duration is not None and max_total_backoff_duration < 0: + raise ValueError('max_total_backoff_duration must be non-negative') + if max_rate_limit_duration is not None and max_rate_limit_duration < 0: + raise ValueError('max_rate_limit_duration must be non-negative') + self.queue = queue.Queue(max_queue_size) self.write_key = write_key self.on_error = on_error @@ -78,6 +87,8 @@ def __init__(self, self.gzip = gzip self.timeout = timeout self.proxies = proxies + self.max_total_backoff_duration = max_total_backoff_duration + self.max_rate_limit_duration = max_rate_limit_duration self.oauth_manager = None if(oauth_client_id and oauth_client_key and oauth_key_id): self.oauth_manager = OauthManager(oauth_client_id, oauth_client_key, oauth_key_id, @@ -110,6 +121,8 @@ def __init__(self, upload_size=upload_size, upload_interval=upload_interval, gzip=gzip, retries=max_retries, timeout=timeout, proxies=proxies, oauth_manager=self.oauth_manager, + max_total_backoff_duration=max_total_backoff_duration, + max_rate_limit_duration=max_rate_limit_duration, ) self.consumers.append(consumer) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 1d822017..e9a9ef7b 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -14,6 +14,10 @@ # lower to leave space for extra data that will be added later, eg. "sentAt". BATCH_SIZE_LIMIT = 475000 +# Default duration limits (12 hours in seconds) +DEFAULT_MAX_TOTAL_BACKOFF_DURATION = 43200 +DEFAULT_MAX_RATE_LIMIT_DURATION = 43200 + class FatalError(Exception): def __init__(self, message): @@ -30,7 +34,9 @@ class Consumer(Thread): def __init__(self, queue, write_key, upload_size=100, host=None, on_error=None, upload_interval=0.5, gzip=False, retries=1000, - timeout=15, proxies=None, oauth_manager=None): + timeout=15, proxies=None, oauth_manager=None, + max_total_backoff_duration=DEFAULT_MAX_TOTAL_BACKOFF_DURATION, + max_rate_limit_duration=DEFAULT_MAX_RATE_LIMIT_DURATION): """Create a consumer thread.""" Thread.__init__(self) # Make consumer a daemon thread so that it doesn't block program exit @@ -51,6 +57,12 @@ def __init__(self, queue, write_key, upload_size=100, host=None, self.timeout = timeout self.proxies = proxies self.oauth_manager = oauth_manager + self.max_total_backoff_duration = max_total_backoff_duration + self.max_rate_limit_duration = max_rate_limit_duration + + # Rate-limit state + self.rate_limited_until = None + self.rate_limit_start_time = None def run(self): """Runs the consumer.""" @@ -64,6 +76,19 @@ def pause(self): """Pause the consumer.""" self.running = False + def set_rate_limit_state(self, response): + """Set rate-limit state from a 429 response with a valid Retry-After header.""" + retry_after = parse_retry_after(response) if response else None + if retry_after: + self.rate_limited_until = time.time() + retry_after + if self.rate_limit_start_time is None: + self.rate_limit_start_time = time.time() + + def clear_rate_limit_state(self): + """Clear rate-limit state after successful request or duration exceeded.""" + self.rate_limited_until = None + self.rate_limit_start_time = None + def upload(self): """Upload the next batch of items, return whether successful.""" success = False @@ -71,9 +96,57 @@ def upload(self): if len(batch) == 0: return False + # Check rate-limit state before attempting upload + if self.rate_limited_until is not None: + now = time.time() + + # Check if maxRateLimitDuration has been exceeded + if (self.rate_limit_start_time is not None and + now - self.rate_limit_start_time > self.max_rate_limit_duration): + self.log.error( + 'Rate limit duration exceeded (%ds). Clearing rate-limit state and dropping batch.', + self.max_rate_limit_duration + ) + self.clear_rate_limit_state() + # Drop the batch by marking items as done + if self.on_error: + self.on_error( + Exception('Rate limit duration exceeded, batch dropped'), + batch + ) + for _ in batch: + self.queue.task_done() + return False + + # Still rate-limited; wait until the rate limit expires + wait_time = self.rate_limited_until - now + if wait_time > 0: + self.log.debug( + 'Rate-limited. Waiting %.2fs before next upload attempt.', + wait_time + ) + time.sleep(wait_time) + try: self.request(batch) + # Success — clear rate-limit state + self.clear_rate_limit_state() success = True + except APIError as e: + if e.status == 429: + # 429: rate-limit state already set by request(). Re-queue batch. + self.log.debug('429 received. Re-queuing batch and halting upload iteration.') + for item in batch: + try: + self.queue.put(item, block=False) + except Exception: + pass # Queue full, item lost + success = False + else: + self.log.error('error uploading: %s', e) + success = False + if self.on_error: + self.on_error(e, batch) except Exception as e: self.log.error('error uploading: %s', e) success = False @@ -128,18 +201,19 @@ def is_retryable_status(status): Retryable 4xx: 408, 410, 429, 460 Non-retryable 4xx: 400, 401, 403, 404, 413, 422, and all other 4xx Retryable 5xx: All except 501, 505 + - 511 is only retryable when OauthManager is configured Non-retryable 5xx: 501, 505 """ if 400 <= status < 500: return status in (408, 410, 429, 460) elif 500 <= status < 600: - return status not in (501, 505) + if status in (501, 505): + return False + if status == 511: + return self.oauth_manager is not None + return True return False - def should_use_retry_after(status): - """Check if status code should respect Retry-After header""" - return status in (408, 429, 503) - def calculate_backoff_delay(attempt): """ Calculate exponential backoff delay with jitter. @@ -153,11 +227,11 @@ def calculate_backoff_delay(attempt): total_attempts = 0 backoff_attempts = 0 - max_backoff_attempts = self.retries + 1 - # Prevent infinite retry loops even with Retry-After - max_total_attempts = max_backoff_attempts * 10 + first_failure_time = None while True: + total_attempts += 1 + try: # Make the request with current retry count response = post( @@ -168,7 +242,7 @@ def calculate_backoff_delay(attempt): batch=batch, proxies=self.proxies, oauth_manager=self.oauth_manager, - retry_count=total_attempts + retry_count=total_attempts - 1 ) # Success return response @@ -179,35 +253,35 @@ def calculate_backoff_delay(attempt): raise except APIError as e: - total_attempts += 1 + # 429 with valid Retry-After: set rate-limit state and raise + # to caller (pipeline blocking). Without Retry-After, fall + # through to counted backoff like any other retryable error. + if e.status == 429: + retry_after = parse_retry_after(e.response) if e.response else None + if retry_after is not None: + self.set_rate_limit_state(e.response) + raise - # Prevent infinite retry loops - if total_attempts >= max_total_attempts: + # Check if status is retryable + if not is_retryable_status(e.status): self.log.error( - f"Maximum total attempts ({max_total_attempts}) reached after {total_attempts} attempts. Final error: {e}" + f"Non-retryable error {e.status} after {total_attempts} attempts: {e}" ) raise - # Check if we should use Retry-After header - if should_use_retry_after(e.status) and e.response: - retry_after = parse_retry_after(e.response) - if retry_after: - self.log.debug( - f"Retry-After header present: waiting {retry_after}s (attempt {total_attempts})" - ) - time.sleep(retry_after) - continue # Does not count against backoff budget - - # Check if status is retryable - if not is_retryable_status(e.status): + # Transient error -- per-batch backoff + if first_failure_time is None: + first_failure_time = time.time() + if time.time() - first_failure_time > self.max_total_backoff_duration: self.log.error( - f"Non-retryable error {e.status} after {total_attempts} attempts: {e}" + f"Max total backoff duration ({self.max_total_backoff_duration}s) exceeded " + f"after {total_attempts} attempts. Final error: {e}" ) raise # Count this against backoff attempts backoff_attempts += 1 - if backoff_attempts >= max_backoff_attempts: + if backoff_attempts >= self.retries + 1: self.log.error( f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" ) @@ -224,17 +298,18 @@ def calculate_backoff_delay(attempt): except Exception as e: # Network errors or other exceptions - retry with backoff - total_attempts += 1 - backoff_attempts += 1 - - # Prevent infinite retry loops - if total_attempts >= max_total_attempts: + if first_failure_time is None: + first_failure_time = time.time() + if time.time() - first_failure_time > self.max_total_backoff_duration: self.log.error( - f"Maximum total attempts ({max_total_attempts}) reached after {total_attempts} attempts. Final error: {e}" + f"Max total backoff duration ({self.max_total_backoff_duration}s) exceeded " + f"after {total_attempts} attempts. Final error: {e}" ) raise - if backoff_attempts >= max_backoff_attempts: + backoff_attempts += 1 + + if backoff_attempts >= self.retries + 1: self.log.error( f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" ) diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index df4b5314..aa9c094d 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -144,7 +144,7 @@ def test_request_retry(self): self._test_request_retry(consumer, APIError( 500, 'code', 'Internal Server Error'), 2) - # we should retry on HTTP 429 errors + # 429 without Retry-After uses counted backoff (like other retryable errors) consumer = Consumer(None, 'testsecret') self._test_request_retry(consumer, APIError( 429, 'code', 'Too Many Requests'), 2) @@ -266,7 +266,7 @@ def mock_post_fn(*args, **kwargs): self.assertEqual(call_count, 1, f'Status {status_code} should not be retried') def test_retryable_4xx_status_codes(self): - """Test that retryable 4xx errors are retried""" + """Test that retryable 4xx errors are retried (429 without Retry-After uses backoff too)""" consumer = Consumer(None, 'testsecret', retries=3) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} @@ -339,107 +339,82 @@ def mock_post_fn(*args, **kwargs): # Should have been called 3 times self.assertEqual(call_count, 3, f'Status {status_code} should be retried') - def test_retry_after_header_support(self): - """Test that Retry-After header is respected and doesn't count against retry budget""" + def test_429_sets_rate_limit_state_with_retry_after(self): + """Test that 429 with Retry-After sets rate_limited_until on consumer""" consumer = Consumer(None, 'testsecret', retries=2) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} - call_count = 0 - sleep_durations = [] - def mock_post_fn(*args, **kwargs): - nonlocal call_count - call_count += 1 - - if call_count <= 3: - # Return 429 with Retry-After for first 3 attempts - response = mock.Mock() - response.headers = {'Retry-After': '10'} - error = APIError(429, 'rate_limit', 'Too Many Requests') - error.response = response - raise error - - # Success on 4th attempt - return mock.Mock(status_code=200) - - def mock_sleep(duration): - sleep_durations.append(duration) + response = mock.Mock() + response.headers = {'Retry-After': '10'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - with mock.patch('time.sleep', side_effect=mock_sleep): + with self.assertRaises(APIError) as ctx: consumer.request([track]) + self.assertEqual(ctx.exception.status, 429) - # Should succeed after 4 attempts (3 Retry-After, then success) - self.assertEqual(call_count, 4) - - # First 3 sleeps should be for Retry-After (10 seconds each) - self.assertEqual(sleep_durations[:3], [10, 10, 10]) + # Rate-limit state should be set + self.assertIsNotNone(consumer.rate_limited_until) + self.assertIsNotNone(consumer.rate_limit_start_time) + # rate_limited_until should be ~10 seconds in the future + self.assertGreater(consumer.rate_limited_until, time.time() + 5) def test_retry_after_capped_at_300_seconds(self): - """Test that Retry-After delay is capped at 300 seconds""" + """Test that Retry-After delay is capped at 300 seconds when setting rate-limit state""" consumer = Consumer(None, 'testsecret', retries=2) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} - call_count = 0 - sleep_duration = None - def mock_post_fn(*args, **kwargs): - nonlocal call_count - call_count += 1 - - if call_count == 1: - # Return 429 with large Retry-After - response = mock.Mock() - response.headers = {'Retry-After': '600'} # 10 minutes - error = APIError(429, 'rate_limit', 'Too Many Requests') - error.response = response - raise error - - # Success on 2nd attempt - return mock.Mock(status_code=200) - - def mock_sleep(duration): - nonlocal sleep_duration - sleep_duration = duration + response = mock.Mock() + response.headers = {'Retry-After': '600'} # 10 minutes + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + now = time.time() with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - with mock.patch('time.sleep', side_effect=mock_sleep): + with self.assertRaises(APIError): consumer.request([track]) - # Sleep should be capped at 300 seconds - self.assertEqual(sleep_duration, 300) + # rate_limited_until should be capped at ~300s from now (not 600s) + self.assertIsNotNone(consumer.rate_limited_until) + self.assertLessEqual(consumer.rate_limited_until, now + 310) + self.assertGreater(consumer.rate_limited_until, now + 290) - def test_retry_after_for_408_and_503(self): - """Test that Retry-After is respected for 408 and 503 status codes""" - consumer = Consumer(None, 'testsecret', retries=2) + def test_408_and_503_use_backoff(self): + """Test that 408 and 503 use exponential backoff""" track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} for status_code in [408, 503]: + consumer = Consumer(None, 'testsecret', retries=2) call_count = 0 - sleep_duration = None + sleep_durations = [] def mock_post_fn(*args, **kwargs): nonlocal call_count call_count += 1 - if call_count == 1: response = mock.Mock() response.headers = {'Retry-After': '5'} error = APIError(status_code, 'error', 'Error') error.response = response raise error - return mock.Mock(status_code=200) def mock_sleep(duration): - nonlocal sleep_duration - sleep_duration = duration + sleep_durations.append(duration) with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): with mock.patch('time.sleep', side_effect=mock_sleep): consumer.request([track]) - self.assertEqual(sleep_duration, 5, f'Retry-After should be respected for {status_code}') + # Should use backoff delay (0 for first retry), NOT the Retry-After value of 5 + self.assertEqual(call_count, 2) + self.assertEqual(len(sleep_durations), 1) + self.assertEqual(sleep_durations[0], 0, f'{status_code} should use backoff, not Retry-After') def test_exponential_backoff_with_jitter(self): """Test that exponential backoff is used for retries without Retry-After""" @@ -536,45 +511,31 @@ def mock_post_fn(*args, **kwargs): # First request should have retry_count=0 self.assertEqual(retry_count, 0) - def test_429_without_retry_after_uses_backoff(self): - """T09: 429 without Retry-After header uses backoff retry""" - consumer = Consumer(None, 'testsecret', retries=3) + def test_429_without_retry_after_uses_counted_backoff(self): + """429 without Retry-After uses counted backoff (not pipeline blocking)""" + consumer = Consumer(None, 'testsecret', retries=2) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} call_count = 0 - retry_counts = [] - sleep_duration = None def mock_post_fn(*args, **kwargs): nonlocal call_count call_count += 1 - retry_counts.append(kwargs.get('retry_count', 0)) - - if call_count == 1: - # 429 without Retry-After header + if call_count < 3: error = APIError(429, 'rate_limit', 'Too Many Requests') error.response = mock.Mock() error.response.headers = {} # No Retry-After raise error - return mock.Mock(status_code=200) - def mock_sleep(duration): - nonlocal sleep_duration - sleep_duration = duration - with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - with mock.patch('time.sleep', side_effect=mock_sleep): + with mock.patch('time.sleep'): consumer.request([track]) - # Should have two attempts - self.assertEqual(call_count, 2) - self.assertEqual(retry_counts, [0, 1]) - - # First retry should be immediate (0s delay) - self.assertIsNotNone(sleep_duration) - if sleep_duration is not None: - self.assertEqual(sleep_duration, 0) + # Should retry with backoff (3 calls: initial + 2 retries) + self.assertEqual(call_count, 3) + # Rate-limit state should NOT be set (no pipeline blocking) + self.assertIsNone(consumer.rate_limited_until) def test_408_without_retry_after_uses_backoff(self): """T10: 408 without Retry-After header uses backoff retry""" @@ -653,71 +614,70 @@ def mock_sleep(duration): if sleep_duration is not None: self.assertEqual(sleep_duration, 0) - def test_511_is_retryable(self): - """T05: 511 status code is retryable (part of 5xx family, not in non-retryable list)""" + def test_511_not_retryable_without_oauth(self): + """T17: 511 is NOT retried when OauthManager is not configured""" consumer = Consumer(None, 'testsecret', retries=3) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} call_count = 0 - retry_counts = [] def mock_post_fn(*args, **kwargs): nonlocal call_count call_count += 1 - retry_counts.append(kwargs.get('retry_count', 0)) + raise APIError(511, 'auth_required', 'Network Authentication Required') + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with self.assertRaises(APIError) as ctx: + consumer.request([track]) + self.assertEqual(ctx.exception.status, 511) + + # Should only be called once (not retried without OAuth) + self.assertEqual(call_count, 1) + + def test_511_retryable_with_oauth(self): + """T17: 511 IS retried when OauthManager is configured""" + oauth_manager = mock.Mock() + consumer = Consumer(None, 'testsecret', retries=3, oauth_manager=oauth_manager) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 if call_count < 3: raise APIError(511, 'auth_required', 'Network Authentication Required') - return mock.Mock(status_code=200) with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - with mock.patch('time.sleep'): # Mock sleep to speed up test + with mock.patch('time.sleep'): consumer.request([track]) - # Should have been called 3 times (511 is retryable) + # Should have been called 3 times (511 is retryable with OAuth) self.assertEqual(call_count, 3) - self.assertEqual(retry_counts, [0, 1, 2]) - def test_retry_after_not_counted_against_backoff_budget(self): - """T17: Retry-After attempts don't consume backoff retry budget""" + def test_429_with_retry_after_does_not_count_against_backoff_budget(self): + """429 with Retry-After raises immediately (pipeline blocking) without consuming backoff budget""" consumer = Consumer(None, 'testsecret', retries=1) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} call_count = 0 - retry_counts = [] def mock_post_fn(*args, **kwargs): nonlocal call_count call_count += 1 - retry_counts.append(kwargs.get('retry_count', 0)) - - if call_count <= 2: - # First two: 429 with Retry-After (shouldn't count against budget) - response = mock.Mock() - response.headers = {'Retry-After': '1'} - error = APIError(429, 'rate_limit', 'Too Many Requests') - error.response = response - raise error - elif call_count == 3: - # Third: 500 without Retry-After (counts against budget) - raise APIError(500, 'error', 'Server Error') - - # Success on 4th attempt - return mock.Mock(status_code=200) + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = mock.Mock() + error.response.headers = {'Retry-After': '1'} + raise error with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - with mock.patch('time.sleep'): # Mock sleep to speed up test + with self.assertRaises(APIError) as ctx: consumer.request([track]) + self.assertEqual(ctx.exception.status, 429) - # Should succeed after 4 attempts: - # - 2 Retry-After attempts (don't count against budget) - # - 1 backoff attempt (counts against budget = 1) - # - 1 final backoff attempt (counts against budget = 1, limit reached) - # Actually wait, with retries=1, we have max_backoff_attempts=2 - # So: 2 Retry-After + 2 backoff attempts = 4 total - self.assertEqual(call_count, 4) - self.assertEqual(retry_counts, [0, 1, 2, 3]) + # 429 with Retry-After raises on first attempt (pipeline blocking) + self.assertEqual(call_count, 1) def test_413_payload_too_large_not_retried(self): """T12: 413 Payload Too Large is non-retryable (won't succeed on retry)""" @@ -739,3 +699,131 @@ def mock_post_fn(*args, **kwargs): # Should only be called once (no retries) self.assertEqual(call_count, 1) + + def test_t04_429_halts_upload_iteration(self): + """T04: 429 halts current upload iteration — batch is re-queued, not dropped""" + q = Queue() + consumer = Consumer(q, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + # Put a message in the queue + q.put(track) + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + response = mock.Mock() + response.headers = {'Retry-After': '10'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + + on_error_called = [] + + def on_error(e, batch): + on_error_called.append((e, batch)) + + consumer.on_error = on_error + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + result = consumer.upload() + + # upload() should return False (not successful) + self.assertFalse(result) + # request() should have been called exactly once + self.assertEqual(call_count, 1) + # on_error should NOT have been called (batch was re-queued, not dropped) + self.assertEqual(len(on_error_called), 0) + # Rate-limit state should be set + self.assertIsNotNone(consumer.rate_limited_until) + self.assertIsNotNone(consumer.rate_limit_start_time) + + def test_t19_max_total_backoff_duration(self): + """T19: Gives up after maxTotalBackoffDuration elapsed""" + consumer = Consumer(None, 'testsecret', retries=1000, + max_total_backoff_duration=5) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + fake_time = [100.0] # Start time + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise APIError(500, 'error', 'Server Error') + + original_time = time.time + + def mock_time(): + # Advance time by 3 seconds on each call after the first + result = fake_time[0] + fake_time[0] += 3.0 + return result + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + with mock.patch('time.time', side_effect=mock_time): + with self.assertRaises(APIError) as ctx: + consumer.request([track]) + self.assertEqual(ctx.exception.status, 500) + + # With max_total_backoff_duration=5 and time advancing 3s per call: + # Attempt 1: fails, first_failure_time set at 100, time now 103 + # Attempt 2: fails, time is 106, 106-100=6 > 5, exceeds duration + # So should be called exactly 2 times + self.assertEqual(call_count, 2) + + def test_t20_max_rate_limit_duration(self): + """T20: Rate-limited state clears and batch is dropped after maxRateLimitDuration""" + q = Queue() + consumer = Consumer(q, 'testsecret', retries=3, + max_rate_limit_duration=10) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + # Pre-set rate-limit state as if we entered it 15 seconds ago + now = time.time() + consumer.rate_limit_start_time = now - 15 # 15s ago, exceeds 10s limit + consumer.rate_limited_until = now + 5 # Would still be rate-limited + + # Put a message in the queue + q.put(track) + + on_error_called = [] + + def on_error(e, batch): + on_error_called.append((e, batch)) + + consumer.on_error = on_error + + # upload() should detect duration exceeded, clear state, drop batch + result = consumer.upload() + + self.assertFalse(result) + # Rate-limit state should be cleared + self.assertIsNone(consumer.rate_limited_until) + self.assertIsNone(consumer.rate_limit_start_time) + # on_error should have been called (batch was dropped) + self.assertEqual(len(on_error_called), 1) + + def test_rate_limit_state_cleared_on_success(self): + """Rate-limit state is cleared after a successful request""" + q = Queue() + consumer = Consumer(q, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + # Set rate-limit state + consumer.rate_limited_until = time.time() - 1 # Already expired + consumer.rate_limit_start_time = time.time() - 10 + + q.put(track) + + with mock.patch('segment.analytics.consumer.post', return_value=mock.Mock(status_code=200)): + result = consumer.upload() + + self.assertTrue(result) + # Rate-limit state should be cleared on success + self.assertIsNone(consumer.rate_limited_until) + self.assertIsNone(consumer.rate_limit_start_time) From a28102fabe4b2ead37662fbb4d22c54c3b98bc79 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 25 Feb 2026 14:01:22 -0500 Subject: [PATCH 07/29] Fix Retry-After: 0 handling and 429 re-queue guard Handle Retry-After: 0 correctly by checking 'is not None' instead of truthiness. Prevent silent batch re-queue on 429 without Retry-After by gating the upload() re-queue path on rate_limited_until being set. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/consumer.py | 4 +- segment/analytics/test/test_consumer.py | 53 +++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index e9a9ef7b..b2143f86 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -79,7 +79,7 @@ def pause(self): def set_rate_limit_state(self, response): """Set rate-limit state from a 429 response with a valid Retry-After header.""" retry_after = parse_retry_after(response) if response else None - if retry_after: + if retry_after is not None: self.rate_limited_until = time.time() + retry_after if self.rate_limit_start_time is None: self.rate_limit_start_time = time.time() @@ -133,7 +133,7 @@ def upload(self): self.clear_rate_limit_state() success = True except APIError as e: - if e.status == 429: + if e.status == 429 and self.rate_limited_until is not None: # 429: rate-limit state already set by request(). Re-queue batch. self.log.debug('429 received. Re-queuing batch and halting upload iteration.') for item in batch: diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index aa9c094d..6fbe9c00 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -741,6 +741,59 @@ def on_error(e, batch): self.assertIsNotNone(consumer.rate_limited_until) self.assertIsNotNone(consumer.rate_limit_start_time) + def test_429_without_retry_after_does_not_requeue_batch(self): + """429 without Retry-After is treated as normal failure in upload() and is not re-queued""" + q = Queue() + consumer = Consumer(q, 'testsecret', retries=0) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + q.put(track) + + def mock_post_fn(*args, **kwargs): + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = mock.Mock() + error.response.headers = {} + raise error + + on_error_called = [] + + def on_error(e, batch): + on_error_called.append((e, batch)) + + consumer.on_error = on_error + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + result = consumer.upload() + + self.assertFalse(result) + self.assertEqual(len(on_error_called), 1) + self.assertIsNone(consumer.rate_limited_until) + self.assertEqual(q.qsize(), 0) + + def test_retry_after_zero_sets_rate_limit_state(self): + """429 with Retry-After: 0 still sets rate-limit state for consistent pipeline handling""" + consumer = Consumer(None, 'testsecret', retries=1) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + def mock_post_fn(*args, **kwargs): + response = mock.Mock() + response.headers = {'Retry-After': '0'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + + before = time.time() + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with self.assertRaises(APIError) as ctx: + consumer.request([track]) + self.assertEqual(ctx.exception.status, 429) + after = time.time() + + self.assertIsNotNone(consumer.rate_limited_until) + self.assertIsNotNone(consumer.rate_limit_start_time) + self.assertGreaterEqual(consumer.rate_limited_until, before) + self.assertLessEqual(consumer.rate_limited_until, after + 0.1) + def test_t19_max_total_backoff_duration(self): """T19: Gives up after maxTotalBackoffDuration elapsed""" consumer = Consumer(None, 'testsecret', retries=1000, From 115d124455a70fb2ce719d470c80ae346b42cb39 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 25 Feb 2026 16:32:53 -0500 Subject: [PATCH 08/29] Address PR review: catch KeyError in response parsing - Add KeyError to except clause when parsing JSON response to handle missing 'code' or 'message' keys - Add explanatory comment on pre-existing except-pass pattern Co-Authored-By: Claude Opus 4.6 --- segment/analytics/request.py | 2 +- segment/analytics/test/test_consumer.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 38a1be99..600fda3b 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -100,7 +100,7 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag payload = res.json() log.debug('received response: %s', payload) raise APIError(res.status_code, payload['code'], payload['message'], res) - except ValueError: + except (ValueError, KeyError): log.error('Unknown error: [%s] %s', res.status_code, res.reason) raise APIError(res.status_code, 'unknown', res.text, res) diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 6fbe9c00..a197843e 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -155,7 +155,7 @@ def test_request_retry(self): try: self._test_request_retry(consumer, api_error, 1) except APIError: - pass + pass # Expected: 400 is non-retryable, so the error propagates here else: self.fail('request() should not retry on client errors') From 2474b4692c8dcded711ade76e6979e66fdf9f8ab Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 25 Feb 2026 17:16:54 -0500 Subject: [PATCH 09/29] Enabling retry e2e test set --- e2e-cli/e2e-config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index b0ccf30c..e1a02d5b 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -1,6 +1,6 @@ { "sdk": "python", - "test_suites": "basic", + "test_suites": "basic,retry", "auto_settings": false, "patch": null, "env": {} From 98df1bcea0d6e82a1761de7361b5258ae4686d48 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 26 Feb 2026 16:48:22 -0500 Subject: [PATCH 10/29] Fix Retry-After header never parsed on non-2xx responses requests.Response.__bool__() returns False for non-2xx status codes. The checks `if e.response` and `if response` evaluated to False for 429 responses, so parse_retry_after() was never called and the SDK fell back to normal backoff instead of respecting Retry-After. Changed both checks to explicit `is not None` comparisons. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/consumer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index b2143f86..adf0565b 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -78,7 +78,7 @@ def pause(self): def set_rate_limit_state(self, response): """Set rate-limit state from a 429 response with a valid Retry-After header.""" - retry_after = parse_retry_after(response) if response else None + retry_after = parse_retry_after(response) if response is not None else None if retry_after is not None: self.rate_limited_until = time.time() + retry_after if self.rate_limit_start_time is None: @@ -257,7 +257,7 @@ def calculate_backoff_delay(attempt): # to caller (pipeline blocking). Without Retry-After, fall # through to counted backoff like any other retryable error. if e.status == 429: - retry_after = parse_retry_after(e.response) if e.response else None + retry_after = parse_retry_after(e.response) if e.response is not None else None if retry_after is not None: self.set_rate_limit_state(e.response) raise From ec0481fb7ee9a3b7ba3927a89e1988e80e00b63b Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 26 Feb 2026 18:20:02 -0500 Subject: [PATCH 11/29] Wire on_error callback in e2e-cli for failure reporting Add on_error handler to capture delivery failures from the SDK. Reports success=false with the first error message when any batch fails (non-retryable error or retries exhausted). Co-Authored-By: Claude Opus 4.6 --- e2e-cli/src/cli.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/e2e-cli/src/cli.py b/e2e-cli/src/cli.py index ef242f6b..d6501c8d 100644 --- a/e2e-cli/src/cli.py +++ b/e2e-cli/src/cli.py @@ -70,6 +70,10 @@ def run(input_json: str, debug: bool): """Run the E2E CLI with the given input configuration.""" logger = setup_logging(debug) output = {"success": False, "sentBatches": 0, "error": None} + delivery_errors = [] + + def on_error(error, batch): + delivery_errors.append(str(error)) try: data = json.loads(input_json) @@ -96,6 +100,7 @@ def run(input_json: str, debug: bool): write_key=write_key, host=api_host, debug=debug, + on_error=on_error, upload_size=flush_at, upload_interval=flush_interval, max_retries=max_retries, @@ -120,10 +125,12 @@ def run(input_json: str, debug: bool): client.flush() client.join() - output["success"] = True - # Note: We don't have easy access to batch count from the SDK internals - # This could be enhanced if needed - output["sentBatches"] = 1 # Placeholder + if delivery_errors: + output["success"] = False + output["error"] = delivery_errors[0] + else: + output["success"] = True + output["sentBatches"] = 1 except json.JSONDecodeError as e: output["error"] = f"Invalid JSON input: {e}" From ea257f8963ea2503d0bc6160daf70a3e00def539 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 3 Mar 2026 20:12:52 -0500 Subject: [PATCH 12/29] Consolidate backoff parameters: max retries 1000 -> 10 Align retry configuration with cross-library defaults. Base backoff (500ms) and max backoff (60s) already matched. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/client.py | 2 +- segment/analytics/consumer.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/segment/analytics/client.py b/segment/analytics/client.py index 5cb09734..6bb0ddd6 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -30,7 +30,7 @@ class DefaultConfig(object): max_queue_size = 10000 gzip = False timeout = 15 - max_retries = 1000 + max_retries = 10 max_total_backoff_duration = 43200 max_rate_limit_duration = 43200 proxies = None diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index adf0565b..1b84bfa7 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -33,7 +33,7 @@ class Consumer(Thread): log = logging.getLogger('segment') def __init__(self, queue, write_key, upload_size=100, host=None, - on_error=None, upload_interval=0.5, gzip=False, retries=1000, + on_error=None, upload_interval=0.5, gzip=False, retries=10, timeout=15, proxies=None, oauth_manager=None, max_total_backoff_duration=DEFAULT_MAX_TOTAL_BACKOFF_DURATION, max_rate_limit_duration=DEFAULT_MAX_RATE_LIMIT_DURATION): From 12d2f085411ac4ed5fceee1f2f9c49666f71eb4c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 20 Mar 2026 12:21:05 -0400 Subject: [PATCH 13/29] Omit X-Retry-Count header on first attempt, send only on retries First request (retry_count=0) no longer includes the header. Retries with retry_count > 0 continue to send X-Retry-Count: 1, 2, 3, etc. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/request.py | 3 ++- segment/analytics/test/test_request.py | 7 +++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 600fda3b..2c7916bc 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -54,8 +54,9 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag headers = { 'Content-Type': 'application/json', 'User-Agent': 'analytics-python/' + VERSION, - 'X-Retry-Count': str(retry_count) } + if retry_count > 0: + headers['X-Retry-Count'] = str(retry_count) # Add Authorization header - prefer OAuth Bearer token, fallback to Basic auth if auth: diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index e0206dda..c5d6b9d7 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -120,14 +120,14 @@ def mock_post_fn(*args, **kwargs): self.assertEqual(headers['Authorization'], 'Bearer test_token_123') def test_x_retry_count_header(self): - """Test that X-Retry-Count header is included""" + """Test that X-Retry-Count header is omitted on first attempt and included on retries""" def mock_post_fn(*args, **kwargs): res = mock.Mock() res.status_code = 200 return res with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn) as mock_post: - # Test with retry_count=0 (first attempt) + # Test with retry_count=0 (first attempt) — header should be absent post('testsecret', retry_count=0, batch=[{ 'userId': 'userId', 'event': 'python event', @@ -136,8 +136,7 @@ def mock_post_fn(*args, **kwargs): args, kwargs = mock_post.call_args headers = kwargs['headers'] - self.assertIn('X-Retry-Count', headers) - self.assertEqual(headers['X-Retry-Count'], '0') + self.assertNotIn('X-Retry-Count', headers) with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn) as mock_post: # Test with retry_count=5 From a2fd03be786e1422a5ba42eec98ee8f75002bbf3 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 1 May 2026 14:03:34 -0400 Subject: [PATCH 14/29] Treat 2xx and 3xx status codes as success, not just 200 Aligns with the analytics-next Node reference implementation which treats all 200-399 responses as successful delivery. Previously only exact 200 was treated as success, causing 201/204/3xx to be misreported as errors. Co-Authored-By: Claude Opus 4.6 --- segment/analytics/request.py | 2 +- segment/analytics/test/test_request.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 2c7916bc..098b536f 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -90,7 +90,7 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag except Exception as e: raise e - if res.status_code == 200: + if 200 <= res.status_code < 400: log.debug('data uploaded successfully') return res diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index c5d6b9d7..4451dd48 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -150,6 +150,22 @@ def mock_post_fn(*args, **kwargs): headers = kwargs['headers'] self.assertEqual(headers['X-Retry-Count'], '5') + def test_non_200_2xx_treated_as_success(self): + """Test that 2xx and 3xx status codes are treated as success""" + for status_code in [200, 201, 204, 301, 302]: + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = status_code + return res + + with mock.patch('segment.analytics.request._session.post', side_effect=mock_post_fn): + res = post('testsecret', batch=[{ + 'userId': 'userId', + 'event': 'python event', + 'type': 'track' + }]) + self.assertEqual(res.status_code, status_code) + def test_parse_retry_after_integer(self): """Test parsing Retry-After header with integer seconds""" response = mock.Mock() From 8c5680a2c43da8b339a56f74a71a140221c2871d Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 7 May 2026 13:08:04 -0400 Subject: [PATCH 15/29] Improve e2e-cli setup: Python resolution and devbox docs - Resolve python via activated venv/devbox, fall back to python3 - Use $PYTHON -m pip instead of bare pip (fixes macOS/nix where pip is not on PATH) - Add devbox setup as recommended path in README, with venv fallback instructions --- e2e-cli/README.md | 30 ++++++++++++++++++++++++------ e2e-cli/run-e2e.sh | 18 +++++++++++++++--- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/e2e-cli/README.md b/e2e-cli/README.md index 28c602a2..1a47b61f 100644 --- a/e2e-cli/README.md +++ b/e2e-cli/README.md @@ -2,17 +2,35 @@ E2E test CLI for the [analytics-python](https://github.com/segmentio/analytics-python) SDK. Accepts a JSON input describing events and SDK configuration, sends them through the real SDK, and outputs results as JSON. -## Setup +## Running E2E tests + +### With devbox (recommended) + +```bash +# From repo root — activates Python 3.12 and installs deps automatically +devbox shell + +# Then from e2e-cli dir: +./run-e2e.sh +``` + +### Without devbox + +Requires Python 3.9+ and Node.js 18+. Using a virtualenv is strongly recommended since macOS system Python is externally managed. ```bash -cd e2e-cli python3 -m venv .venv source .venv/bin/activate -pip install -r requirements.txt -pip install -e . +./run-e2e.sh +``` + +### Override sdk-e2e-tests location + +```bash +E2E_TESTS_DIR=../my-e2e-tests ./run-e2e.sh ``` -## Usage +## Manual CLI usage ```bash e2e-cli --input '{"writeKey":"...", ...}' @@ -21,7 +39,7 @@ e2e-cli --input '{"writeKey":"...", ...}' Or without installing: ```bash -python3 -m src.cli --input '{"writeKey":"...", ...}' +python3 src/cli.py --input '{"writeKey":"...", ...}' ``` ## Input Format diff --git a/e2e-cli/run-e2e.sh b/e2e-cli/run-e2e.sh index 533dba9e..a90b5204 100755 --- a/e2e-cli/run-e2e.sh +++ b/e2e-cli/run-e2e.sh @@ -2,7 +2,9 @@ # # Run E2E tests for analytics-python # -# Prerequisites: Python 3, pip, Node.js 18+ +# Prerequisites: Node.js 18+ and one of: +# - devbox (recommended): run `devbox shell` first, then ./run-e2e.sh +# - Python 3.9+ with a virtualenv already activated # # Usage: # ./run-e2e.sh [extra args passed to run-tests.sh] @@ -17,15 +19,25 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" SDK_ROOT="$SCRIPT_DIR/.." E2E_DIR="${E2E_TESTS_DIR:-$SDK_ROOT/../sdk-e2e-tests}" +# Resolve python and pip — prefer activated venv/devbox python, fall back to python3 +PYTHON="${PYTHON:-$(command -v python || command -v python3)}" +PIP="$PYTHON -m pip" + +if [[ -z "$PYTHON" ]]; then + echo "Error: Python not found. Run 'devbox shell' first or activate a virtualenv." + exit 1 +fi + echo "=== Building analytics-python e2e-cli ===" +echo "Using Python: $PYTHON" # Install SDK cd "$SDK_ROOT" -pip install -e . +$PIP install -e . -q # Install e2e-cli cd "$SCRIPT_DIR" -pip install -e . +$PIP install -e . -q echo "" From ad198ef8e60295d84bc3d71e4d20f56c67c4a37b Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Mon, 11 May 2026 16:27:47 -0400 Subject: [PATCH 16/29] Address all issues from deep code review - Remove unused backoff dependency from setup.py and requirements.txt - Reject None for max_total_backoff_duration/max_rate_limit_duration in client.py - Fix off-by-one: use >= for max_total_backoff_duration check - Fix Retry-After: 0 tight loop: fall through to counted backoff instead of pipeline-blocking - Log and call on_error when queue is full during 429 re-queue - Document flush() blocking behavior in docstring - Add comment explaining 410 retryable parity with Node SDK - Extract duplicate backoff logic into apply_backoff() helper - Warn on unrecognized Retry-After format (HTTP-date) instead of silently ignoring - Add comments for task_done() invariant and FatalError origin - Add 8 new tests covering all previously missing coverage gaps --- requirements.txt | 1 - segment/analytics/client.py | 15 +- segment/analytics/consumer.py | 125 +++++++--------- segment/analytics/request.py | 8 +- segment/analytics/test/test_consumer.py | 185 ++++++++++++++++++++++-- setup.py | 1 - status-response-updates-deep-review.md | 112 ++++++++++++++ 7 files changed, 346 insertions(+), 101 deletions(-) create mode 100644 status-response-updates-deep-review.md diff --git a/requirements.txt b/requirements.txt index 596c10da..912848b9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,3 @@ -backoff==2.2.1 cryptography==44.0.0 flake8==7.1.1 mock==2.0.0 diff --git a/segment/analytics/client.py b/segment/analytics/client.py index 6bb0ddd6..9a8dcdb9 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -72,10 +72,10 @@ def __init__(self, max_rate_limit_duration=DefaultConfig.max_rate_limit_duration,): require('write_key', write_key, str) - if max_total_backoff_duration is not None and max_total_backoff_duration < 0: - raise ValueError('max_total_backoff_duration must be non-negative') - if max_rate_limit_duration is not None and max_rate_limit_duration < 0: - raise ValueError('max_rate_limit_duration must be non-negative') + if max_total_backoff_duration is None or max_total_backoff_duration < 0: + raise ValueError('max_total_backoff_duration must be a non-negative number') + if max_rate_limit_duration is None or max_rate_limit_duration < 0: + raise ValueError('max_rate_limit_duration must be a non-negative number') self.queue = queue.Queue(max_queue_size) self.write_key = write_key @@ -331,7 +331,12 @@ def _enqueue(self, msg): return False, msg def flush(self): - """Forces a flush from the internal queue to the server""" + """Forces a flush from the internal queue to the server. + + Warning: if the consumer is currently rate-limited, this call will + block until the rate limit clears or max_rate_limit_duration elapses + (up to 12 hours by default). + """ queue = self.queue size = queue.qsize() queue.join() diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 1b84bfa7..3748ba19 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -136,11 +136,16 @@ def upload(self): if e.status == 429 and self.rate_limited_until is not None: # 429: rate-limit state already set by request(). Re-queue batch. self.log.debug('429 received. Re-queuing batch and halting upload iteration.') + dropped = [] for item in batch: try: self.queue.put(item, block=False) except Exception: - pass # Queue full, item lost + dropped.append(item) + if dropped: + self.log.error('Queue full during 429 re-queue. Dropping %d item(s).', len(dropped)) + if self.on_error: + self.on_error(Exception('Queue full, items dropped during 429 re-queue'), dropped) success = False else: self.log.error('error uploading: %s', e) @@ -153,7 +158,9 @@ def upload(self): if self.on_error: self.on_error(e, batch) finally: - # mark items as acknowledged from queue + # Each item in batch was obtained via queue.get() and must have + # exactly one matching task_done() call — including re-queued items, + # which will produce a new task_done() obligation on their next get(). for _ in batch: self.queue.task_done() return success @@ -196,14 +203,13 @@ def request(self, batch): """Attempt to upload the batch and retry before raising an error""" def is_retryable_status(status): - """ - Determine if a status code is retryable. - Retryable 4xx: 408, 410, 429, 460 - Non-retryable 4xx: 400, 401, 403, 404, 413, 422, and all other 4xx - Retryable 5xx: All except 501, 505 - - 511 is only retryable when OauthManager is configured - Non-retryable 5xx: 501, 505 - """ + # Retryable 4xx: 408, 429, 460 + # 410 Gone: permanently removed, but included for parity with the + # Node.js SDK. Retrying is harmless since the server will keep + # returning 410, and the retry budget caps total attempts. + # Non-retryable 4xx: 400, 401, 403, 404, 413, 422, and all other 4xx + # Retryable 5xx: all except 501, 505 + # 511: only retryable when OauthManager is configured if 400 <= status < 500: return status in (408, 410, 429, 460) elif 500 <= status < 600: @@ -215,15 +221,36 @@ def is_retryable_status(status): return False def calculate_backoff_delay(attempt): - """ - Calculate exponential backoff delay with jitter. - First retry is immediate, then 0.5s, 1s, 2s, 4s, etc. - """ + # First retry is immediate; thereafter 0.5s, 1s, 2s, 4s… capped at 60s if attempt == 1: - return 0 # First retry is immediate + return 0 base_delay = 0.5 * (2 ** (attempt - 2)) jitter = random.uniform(0, 0.1 * base_delay) - return min(base_delay + jitter, 60) # Cap at 60 seconds + return min(base_delay + jitter, 60) + + def apply_backoff(e, label): + """Apply retry backoff logic. Returns delay if should retry, raises if exhausted.""" + nonlocal first_failure_time, backoff_attempts + if first_failure_time is None: + first_failure_time = time.time() + if time.time() - first_failure_time >= self.max_total_backoff_duration: + self.log.error( + f"Max total backoff duration ({self.max_total_backoff_duration}s) exceeded " + f"after {total_attempts} attempts. Final error: {e}" + ) + raise e + backoff_attempts += 1 + if backoff_attempts >= self.retries + 1: + self.log.error( + f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" + ) + raise e + delay = calculate_backoff_delay(backoff_attempts) + self.log.debug( + f"{label} {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " + f"after {delay:.2f}s: {e}" + ) + return delay total_attempts = 0 backoff_attempts = 0 @@ -233,7 +260,6 @@ def calculate_backoff_delay(attempt): total_attempts += 1 try: - # Make the request with current retry count response = post( self.write_key, self.host, @@ -244,82 +270,33 @@ def calculate_backoff_delay(attempt): oauth_manager=self.oauth_manager, retry_count=total_attempts - 1 ) - # Success return response except FatalError as e: - # Non-retryable error + # Raised by oauth_manager when token refresh fails permanently; + # not safe to retry. self.log.error(f"Fatal error after {total_attempts} attempts: {e}") raise except APIError as e: - # 429 with valid Retry-After: set rate-limit state and raise - # to caller (pipeline blocking). Without Retry-After, fall - # through to counted backoff like any other retryable error. + # 429 with valid Retry-After > 0: block the pipeline and let + # upload() re-queue the batch. Retry-After: 0 or missing falls + # through to counted backoff to avoid a tight re-queue loop. if e.status == 429: retry_after = parse_retry_after(e.response) if e.response is not None else None - if retry_after is not None: + if retry_after is not None and retry_after > 0: self.set_rate_limit_state(e.response) raise - # Check if status is retryable if not is_retryable_status(e.status): self.log.error( f"Non-retryable error {e.status} after {total_attempts} attempts: {e}" ) raise - # Transient error -- per-batch backoff - if first_failure_time is None: - first_failure_time = time.time() - if time.time() - first_failure_time > self.max_total_backoff_duration: - self.log.error( - f"Max total backoff duration ({self.max_total_backoff_duration}s) exceeded " - f"after {total_attempts} attempts. Final error: {e}" - ) - raise - - # Count this against backoff attempts - backoff_attempts += 1 - if backoff_attempts >= self.retries + 1: - self.log.error( - f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" - ) - raise - - # Calculate exponential backoff delay with jitter - delay = calculate_backoff_delay(backoff_attempts) - - self.log.debug( - f"Retry attempt {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " - f"after {delay:.2f}s for status {e.status}" - ) + delay = apply_backoff(e, f"Retry attempt (status {e.status})") time.sleep(delay) except Exception as e: - # Network errors or other exceptions - retry with backoff - if first_failure_time is None: - first_failure_time = time.time() - if time.time() - first_failure_time > self.max_total_backoff_duration: - self.log.error( - f"Max total backoff duration ({self.max_total_backoff_duration}s) exceeded " - f"after {total_attempts} attempts. Final error: {e}" - ) - raise - - backoff_attempts += 1 - - if backoff_attempts >= self.retries + 1: - self.log.error( - f"All {self.retries} retries exhausted after {total_attempts} total attempts. Final error: {e}" - ) - raise - - # Calculate exponential backoff delay with jitter - delay = calculate_backoff_delay(backoff_attempts) - - self.log.debug( - f"Network error retry {backoff_attempts}/{self.retries} (total attempts: {total_attempts}) " - f"after {delay:.2f}s: {e}" - ) + delay = apply_backoff(e, "Network error retry") time.sleep(delay) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 098b536f..8fb538de 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -28,13 +28,13 @@ def parse_retry_after(response): return None try: - # Try parsing as integer (delay in seconds) delay = int(retry_after) - # Ensure delay is non-negative before applying upper bound return min(max(delay, 0), MAX_RETRY_AFTER_SECONDS) except ValueError: - # Could be HTTP-date format, but for simplicity we'll skip that - # Most APIs use integer seconds + # RFC 7231 allows HTTP-date format (e.g. "Wed, 21 Oct 2015 07:28:00 GMT") + # but we don't parse it; fall back to counted backoff. + log = logging.getLogger('segment') + log.warning('Unrecognized Retry-After format %r; ignoring header.', retry_after) return None diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index a197843e..3419bb0f 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -770,29 +770,33 @@ def on_error(e, batch): self.assertIsNone(consumer.rate_limited_until) self.assertEqual(q.qsize(), 0) - def test_retry_after_zero_sets_rate_limit_state(self): - """429 with Retry-After: 0 still sets rate-limit state for consistent pipeline handling""" - consumer = Consumer(None, 'testsecret', retries=1) + def test_retry_after_zero_uses_counted_backoff(self): + """429 with Retry-After: 0 falls through to counted backoff (not pipeline blocking). + Prevents a tight re-queue loop when the server says retry immediately.""" + consumer = Consumer(None, 'testsecret', retries=2) track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + call_count = 0 + def mock_post_fn(*args, **kwargs): - response = mock.Mock() - response.headers = {'Retry-After': '0'} - error = APIError(429, 'rate_limit', 'Too Many Requests') - error.response = response - raise error + nonlocal call_count + call_count += 1 + if call_count < 3: + response = mock.Mock() + response.headers = {'Retry-After': '0'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + return mock.Mock(status_code=200) - before = time.time() with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): - with self.assertRaises(APIError) as ctx: + with mock.patch('time.sleep'): consumer.request([track]) - self.assertEqual(ctx.exception.status, 429) - after = time.time() - self.assertIsNotNone(consumer.rate_limited_until) - self.assertIsNotNone(consumer.rate_limit_start_time) - self.assertGreaterEqual(consumer.rate_limited_until, before) - self.assertLessEqual(consumer.rate_limited_until, after + 0.1) + # Should retry with backoff (3 calls: initial + 2 retries) + self.assertEqual(call_count, 3) + # Rate-limit state must NOT be set (no pipeline blocking for Retry-After: 0) + self.assertIsNone(consumer.rate_limited_until) def test_t19_max_total_backoff_duration(self): """T19: Gives up after maxTotalBackoffDuration elapsed""" @@ -880,3 +884,152 @@ def test_rate_limit_state_cleared_on_success(self): # Rate-limit state should be cleared on success self.assertIsNone(consumer.rate_limited_until) self.assertIsNone(consumer.rate_limit_start_time) + + def test_retry_after_zero_does_not_trigger_pipeline_blocking(self): + """Retry-After: 0 must not cause tight re-queue loop; falls through to counted backoff""" + q = Queue() + consumer = Consumer(q, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + q.put(track) + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + response = mock.Mock() + response.headers = {'Retry-After': '0'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + result = consumer.upload() + + # Should succeed on retry (counted backoff path, not pipeline-blocking) + self.assertTrue(result) + # Rate-limit state must NOT be set (no pipeline blocking) + self.assertIsNone(consumer.rate_limited_until) + + def test_queue_full_during_429_requeue_calls_on_error(self): + """Queue-full during 429 re-queue calls on_error with dropped items""" + from queue import Full + q = Queue() + consumer = Consumer(q, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + q.put(track) + + dropped_batches = [] + + def on_error(e, batch): + dropped_batches.append(batch) + + consumer.on_error = on_error + + def mock_post_fn(*args, **kwargs): + response = mock.Mock() + response.headers = {'Retry-After': '5'} + error = APIError(429, 'rate_limit', 'Too Many Requests') + error.response = response + raise error + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + # Make queue.put raise Full to simulate a full queue + with mock.patch.object(q, 'put', side_effect=Full): + result = consumer.upload() + + self.assertFalse(result) + # on_error should have been called with the dropped item + self.assertEqual(len(dropped_batches), 1) + self.assertEqual(len(dropped_batches[0]), 1) + + def test_none_max_total_backoff_duration_rejected_by_client(self): + """Client rejects None for max_total_backoff_duration""" + from segment.analytics.client import Client + with self.assertRaises(ValueError): + Client('testsecret', max_total_backoff_duration=None) + + def test_none_max_rate_limit_duration_rejected_by_client(self): + """Client rejects None for max_rate_limit_duration""" + from segment.analytics.client import Client + with self.assertRaises(ValueError): + Client('testsecret', max_rate_limit_duration=None) + + def test_max_total_backoff_duration_zero_prevents_retry(self): + """max_total_backoff_duration=0 prevents any retry attempt""" + consumer = Consumer(None, 'testsecret', retries=1000, + max_total_backoff_duration=0) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise APIError(500, 'error', 'Server Error') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + with self.assertRaises(APIError): + consumer.request([track]) + + # With duration=0 and >= check, first failure sets first_failure_time + # and immediately satisfies time.time() - first_failure_time >= 0, + # so it raises on the very first failure (1 attempt total). + self.assertEqual(call_count, 1) + + def test_parse_retry_after_http_date_logs_warning(self): + """parse_retry_after logs a warning for HTTP-date format and returns None""" + import logging + from segment.analytics.request import parse_retry_after + + response = mock.Mock() + response.headers = {'Retry-After': 'Wed, 21 Oct 2015 07:28:00 GMT'} + + with self.assertLogs('segment', level=logging.WARNING) as cm: + result = parse_retry_after(response) + + self.assertIsNone(result) + self.assertTrue(any('Unrecognized Retry-After' in line for line in cm.output)) + + def test_410_and_460_retried(self): + """410 and 460 are retryable status codes""" + for status_code in [410, 460]: + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + call_count = 0 + + def mock_post_fn(*args, _status=status_code, **kwargs): + nonlocal call_count + call_count += 1 + if call_count < 2: + raise APIError(_status, 'error', f'Error {_status}') + return mock.Mock(status_code=200) + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + consumer.request([track]) + + self.assertEqual(call_count, 2, f'{status_code} should be retried') + + def test_505_not_retried(self): + """505 HTTP Version Not Supported is non-retryable""" + consumer = Consumer(None, 'testsecret', retries=3) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + call_count = 0 + + def mock_post_fn(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise APIError(505, 'error', 'HTTP Version Not Supported') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with self.assertRaises(APIError) as ctx: + consumer.request([track]) + self.assertEqual(ctx.exception.status, 505) + + self.assertEqual(call_count, 1) diff --git a/setup.py b/setup.py index c8ab0a3e..c8bb56cc 100644 --- a/setup.py +++ b/setup.py @@ -21,7 +21,6 @@ install_requires = [ "requests~=2.7", - "backoff~=2.1", "python-dateutil~=2.2", "PyJWT~=2.12" ] diff --git a/status-response-updates-deep-review.md b/status-response-updates-deep-review.md new file mode 100644 index 00000000..4ffdef92 --- /dev/null +++ b/status-response-updates-deep-review.md @@ -0,0 +1,112 @@ +# Deep Code Review — analytics-python `response-status-updates` + +PR: https://github.com/segmentio/analytics-python/pull/520 + +--- + +## 🔴 Critical / Must-Fix + +**1. `backoff` dependency not removed** + +`setup.py` and `requirements.txt` still declare `backoff~=2.1` / `backoff==2.2.1` even though the import was removed from `consumer.py`. Every user still gets an unused transitive dependency shipped to them. + +> ✅ **Resolved**: Removed `backoff~=2.1` from `setup.py` `install_requires` and `backoff==2.2.1` from `requirements.txt`. + +--- + +**2. `None` values for `max_total_backoff_duration` / `max_rate_limit_duration` cause `TypeError`** + +`client.py` accepts `None` (only rejects negative values), but `consumer.py` does unguarded arithmetic comparisons against these values. Passing `None` will raise a `TypeError` at runtime. + +> ✅ **Resolved**: `client.py` validation now rejects `None` with a `ValueError` (same branch as the negative-value check). Tests added: `test_none_max_total_backoff_duration_rejected_by_client` and `test_none_max_rate_limit_duration_rejected_by_client`. + +--- + +## 🟠 Important / Should-Fix + +**3. `max_total_backoff_duration=0` allows one extra attempt** + +The duration check uses `>` rather than `>=`, and `first_failure_time` is set on the same line as the check, creating a race where the first failure always slips through even with a duration of 0. + +> ✅ **Resolved**: Changed `>` to `>=` in both `APIError` and generic `Exception` backoff branches. With `duration=0`, the first failure sets `first_failure_time` and immediately satisfies the check (`0 >= 0`), so the request raises after 1 attempt. Test added: `test_max_total_backoff_duration_zero_prevents_retry`. + +--- + +**4. `Retry-After: 0` causes a tight re-queue loop** + +`parse_retry_after` returns `0`, which sets `rate_limited_until` to "now already expired", causing upload to immediately re-attempt with no delay, re-queue on 429, and loop tight. + +> ✅ **Resolved**: `request()` now only triggers pipeline-blocking (set rate-limit state + raise) when `retry_after > 0`. A `Retry-After: 0` header falls through to counted backoff. Tests updated: `test_retry_after_zero_sets_rate_limit_state` renamed and rewritten as `test_retry_after_zero_uses_counted_backoff`; `test_retry_after_zero_does_not_trigger_pipeline_blocking` added for the `upload()` path. + +--- + +**5. Queue full during 429 re-queue silently drops items** + +`except Exception: pass` in the re-queue loop swallows queue-full errors with no log and no `on_error` callback. Items are dropped silently. + +> ✅ **Resolved**: Collects dropped items, logs an error with the count, and calls `on_error` if configured. Test added: `test_queue_full_during_429_requeue_calls_on_error`. + +--- + +**6. `flush()` can block for up to 12 hours** + +When rate-limited with pipeline-blocking 429s, `queue.join()` in `flush()` waits for all pending `task_done()` calls, which can take up to `max_rate_limit_duration`. This is intentional but undocumented. + +> ✅ **Resolved**: Added a docstring warning on `flush()` documenting the blocking behavior and worst-case duration. + +--- + +**7. `410 Gone` is marked retryable** + +HTTP 410 means permanently removed; retrying will never succeed with the same payload. The Node reference does it too, but there should be a code comment explaining the rationale, or it should be removed. + +> ✅ **Resolved**: Added an inline comment in `is_retryable_status` explaining that 410 is included for parity with the Node.js SDK, and that the retry budget caps total attempts. Test added: `test_410_and_460_retried`. + +--- + +## 🟡 Minor / Nitpick + +**8. Duplicate backoff code in two `except` branches** + +The backoff delay calculation is duplicated in two `except` branches of `request()`. Should be extracted to a helper to prevent drift. + +> ✅ **Resolved**: Extracted shared duration-check + retry-count-check + delay-calc + log into an `apply_backoff(e, label)` inner function. Both `APIError` and generic `Exception` branches now call it. + +--- + +**9. `Retry-After` HTTP-date format silently falls back with no warning** + +RFC 7231 allows `Retry-After` to be either a delay-seconds integer or an HTTP-date string. When an HTTP-date is received, `parse_retry_after` silently falls back to counted backoff with no warning log. No test covers this path. + +> ✅ **Resolved**: `parse_retry_after` now logs `WARNING: Unrecognized Retry-After format ...; ignoring header.` on `ValueError`. Test added: `test_parse_retry_after_http_date_logs_warning`. + +--- + +**10. `task_done()` separation between early-return and `finally` is fragile** + +The placement of `task_done()` calls relative to early returns needs a comment explaining the invariant, otherwise it's easy to introduce a double-call or missed-call on future edits. + +> ✅ **Resolved**: Added a comment above the `finally` block explaining the invariant: each item obtained via `queue.get()` must have exactly one `task_done()`, including re-queued items (which incur a new obligation on their next `get()`). + +--- + +**11. `FatalError` catch is non-obvious** + +The `FatalError` catch in `consumer.py` is non-obvious without reading `oauth_manager.py`. Add a comment explaining what raises it and why it should be terminal. + +> ✅ **Resolved**: Added an inline comment: "Raised by oauth_manager when token refresh fails permanently; not safe to retry." + +--- + +## Test Coverage Gaps + +| Status | Gap | +|--------|-----| +| ✅ Added | `Retry-After: 0` tight-loop behavior | +| ✅ Added | Queue-full during 429 re-queue (silent drop) | +| ✅ Added | `None` passed for `max_total_backoff_duration` / `max_rate_limit_duration` | +| ✅ Added | `parse_retry_after` with HTTP-date format input | +| ✅ Added | `max_total_backoff_duration=0` off-by-one (first failure always passes) | +| ✅ Added | 410 and 460 retryable | +| ✅ Added | 505 non-retryable 5xx | +| ⚠️ Untested | `flush()` blocking behavior during active rate-limit | From 211f094a75e8c666989e78fd31c4fbcfd95eca0c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Mon, 11 May 2026 16:34:45 -0400 Subject: [PATCH 17/29] Refine documentation in flush method Removed unnecessary line in flush method documentation. --- segment/analytics/client.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/segment/analytics/client.py b/segment/analytics/client.py index 9a8dcdb9..e1ffb98a 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -334,8 +334,7 @@ def flush(self): """Forces a flush from the internal queue to the server. Warning: if the consumer is currently rate-limited, this call will - block until the rate limit clears or max_rate_limit_duration elapses - (up to 12 hours by default). + block until the rate limit clears or max_rate_limit_duration elapses. """ queue = self.queue size = queue.qsize() From a1314f8bc8efd160b7504e4b870a035c124afc7f Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Mon, 11 May 2026 16:38:51 -0400 Subject: [PATCH 18/29] Delete status-response-updates-deep-review.md --- status-response-updates-deep-review.md | 112 ------------------------- 1 file changed, 112 deletions(-) delete mode 100644 status-response-updates-deep-review.md diff --git a/status-response-updates-deep-review.md b/status-response-updates-deep-review.md deleted file mode 100644 index 4ffdef92..00000000 --- a/status-response-updates-deep-review.md +++ /dev/null @@ -1,112 +0,0 @@ -# Deep Code Review — analytics-python `response-status-updates` - -PR: https://github.com/segmentio/analytics-python/pull/520 - ---- - -## 🔴 Critical / Must-Fix - -**1. `backoff` dependency not removed** - -`setup.py` and `requirements.txt` still declare `backoff~=2.1` / `backoff==2.2.1` even though the import was removed from `consumer.py`. Every user still gets an unused transitive dependency shipped to them. - -> ✅ **Resolved**: Removed `backoff~=2.1` from `setup.py` `install_requires` and `backoff==2.2.1` from `requirements.txt`. - ---- - -**2. `None` values for `max_total_backoff_duration` / `max_rate_limit_duration` cause `TypeError`** - -`client.py` accepts `None` (only rejects negative values), but `consumer.py` does unguarded arithmetic comparisons against these values. Passing `None` will raise a `TypeError` at runtime. - -> ✅ **Resolved**: `client.py` validation now rejects `None` with a `ValueError` (same branch as the negative-value check). Tests added: `test_none_max_total_backoff_duration_rejected_by_client` and `test_none_max_rate_limit_duration_rejected_by_client`. - ---- - -## 🟠 Important / Should-Fix - -**3. `max_total_backoff_duration=0` allows one extra attempt** - -The duration check uses `>` rather than `>=`, and `first_failure_time` is set on the same line as the check, creating a race where the first failure always slips through even with a duration of 0. - -> ✅ **Resolved**: Changed `>` to `>=` in both `APIError` and generic `Exception` backoff branches. With `duration=0`, the first failure sets `first_failure_time` and immediately satisfies the check (`0 >= 0`), so the request raises after 1 attempt. Test added: `test_max_total_backoff_duration_zero_prevents_retry`. - ---- - -**4. `Retry-After: 0` causes a tight re-queue loop** - -`parse_retry_after` returns `0`, which sets `rate_limited_until` to "now already expired", causing upload to immediately re-attempt with no delay, re-queue on 429, and loop tight. - -> ✅ **Resolved**: `request()` now only triggers pipeline-blocking (set rate-limit state + raise) when `retry_after > 0`. A `Retry-After: 0` header falls through to counted backoff. Tests updated: `test_retry_after_zero_sets_rate_limit_state` renamed and rewritten as `test_retry_after_zero_uses_counted_backoff`; `test_retry_after_zero_does_not_trigger_pipeline_blocking` added for the `upload()` path. - ---- - -**5. Queue full during 429 re-queue silently drops items** - -`except Exception: pass` in the re-queue loop swallows queue-full errors with no log and no `on_error` callback. Items are dropped silently. - -> ✅ **Resolved**: Collects dropped items, logs an error with the count, and calls `on_error` if configured. Test added: `test_queue_full_during_429_requeue_calls_on_error`. - ---- - -**6. `flush()` can block for up to 12 hours** - -When rate-limited with pipeline-blocking 429s, `queue.join()` in `flush()` waits for all pending `task_done()` calls, which can take up to `max_rate_limit_duration`. This is intentional but undocumented. - -> ✅ **Resolved**: Added a docstring warning on `flush()` documenting the blocking behavior and worst-case duration. - ---- - -**7. `410 Gone` is marked retryable** - -HTTP 410 means permanently removed; retrying will never succeed with the same payload. The Node reference does it too, but there should be a code comment explaining the rationale, or it should be removed. - -> ✅ **Resolved**: Added an inline comment in `is_retryable_status` explaining that 410 is included for parity with the Node.js SDK, and that the retry budget caps total attempts. Test added: `test_410_and_460_retried`. - ---- - -## 🟡 Minor / Nitpick - -**8. Duplicate backoff code in two `except` branches** - -The backoff delay calculation is duplicated in two `except` branches of `request()`. Should be extracted to a helper to prevent drift. - -> ✅ **Resolved**: Extracted shared duration-check + retry-count-check + delay-calc + log into an `apply_backoff(e, label)` inner function. Both `APIError` and generic `Exception` branches now call it. - ---- - -**9. `Retry-After` HTTP-date format silently falls back with no warning** - -RFC 7231 allows `Retry-After` to be either a delay-seconds integer or an HTTP-date string. When an HTTP-date is received, `parse_retry_after` silently falls back to counted backoff with no warning log. No test covers this path. - -> ✅ **Resolved**: `parse_retry_after` now logs `WARNING: Unrecognized Retry-After format ...; ignoring header.` on `ValueError`. Test added: `test_parse_retry_after_http_date_logs_warning`. - ---- - -**10. `task_done()` separation between early-return and `finally` is fragile** - -The placement of `task_done()` calls relative to early returns needs a comment explaining the invariant, otherwise it's easy to introduce a double-call or missed-call on future edits. - -> ✅ **Resolved**: Added a comment above the `finally` block explaining the invariant: each item obtained via `queue.get()` must have exactly one `task_done()`, including re-queued items (which incur a new obligation on their next `get()`). - ---- - -**11. `FatalError` catch is non-obvious** - -The `FatalError` catch in `consumer.py` is non-obvious without reading `oauth_manager.py`. Add a comment explaining what raises it and why it should be terminal. - -> ✅ **Resolved**: Added an inline comment: "Raised by oauth_manager when token refresh fails permanently; not safe to retry." - ---- - -## Test Coverage Gaps - -| Status | Gap | -|--------|-----| -| ✅ Added | `Retry-After: 0` tight-loop behavior | -| ✅ Added | Queue-full during 429 re-queue (silent drop) | -| ✅ Added | `None` passed for `max_total_backoff_duration` / `max_rate_limit_duration` | -| ✅ Added | `parse_retry_after` with HTTP-date format input | -| ✅ Added | `max_total_backoff_duration=0` off-by-one (first failure always passes) | -| ✅ Added | 410 and 460 retryable | -| ✅ Added | 505 non-retryable 5xx | -| ⚠️ Untested | `flush()` blocking behavior during active rate-limit | From 9818b3418e77f5f0fdde736c369c486931f855c4 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 2 Sep 2026 19:38:14 -0400 Subject: [PATCH 19/29] Handle Retry-After on every retryable status, including 529 Route any retryable response carrying a valid Retry-After header through the rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable statuses without Retry-After continue to use counted exponential backoff. Adds 529 to the retryable set and covers both paths with tests. Matches the behaviour already shipped in analytics-java 3.5.5 and the generic-retry-after conformance suite in sdk-e2e-tests. --- segment/analytics/consumer.py | 25 +++---- segment/analytics/request.py | 14 +++- segment/analytics/test/test_client.py | 26 ++++++- segment/analytics/test/test_consumer.py | 90 +++++++++++++++++++++---- segment/analytics/test/test_request.py | 26 ++++++- 5 files changed, 150 insertions(+), 31 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 3748ba19..4dc14f3d 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -133,9 +133,8 @@ def upload(self): self.clear_rate_limit_state() success = True except APIError as e: - if e.status == 429 and self.rate_limited_until is not None: - # 429: rate-limit state already set by request(). Re-queue batch. - self.log.debug('429 received. Re-queuing batch and halting upload iteration.') + if self.rate_limited_until is not None: + self.log.debug('Rate-limited (status %d). Re-queuing batch and halting upload iteration.', e.status) dropped = [] for item in batch: try: @@ -143,9 +142,9 @@ def upload(self): except Exception: dropped.append(item) if dropped: - self.log.error('Queue full during 429 re-queue. Dropping %d item(s).', len(dropped)) + self.log.error('Queue full during rate-limit re-queue. Dropping %d item(s).', len(dropped)) if self.on_error: - self.on_error(Exception('Queue full, items dropped during 429 re-queue'), dropped) + self.on_error(Exception('Queue full, items dropped during rate-limit re-queue'), dropped) success = False else: self.log.error('error uploading: %s', e) @@ -279,21 +278,19 @@ def apply_backoff(e, label): raise except APIError as e: - # 429 with valid Retry-After > 0: block the pipeline and let - # upload() re-queue the batch. Retry-After: 0 or missing falls - # through to counted backoff to avoid a tight re-queue loop. - if e.status == 429: - retry_after = parse_retry_after(e.response) if e.response is not None else None - if retry_after is not None and retry_after > 0: - self.set_rate_limit_state(e.response) - raise - if not is_retryable_status(e.status): self.log.error( f"Non-retryable error {e.status} after {total_attempts} attempts: {e}" ) raise + # Any retryable status with valid Retry-After > 0: block pipeline, re-queue + retry_after = parse_retry_after(e.response) if e.response is not None else None + if retry_after is not None and retry_after > 0: + self.set_rate_limit_state(e.response) + raise + + # No Retry-After: counted backoff delay = apply_backoff(e, f"Retry attempt (status {e.status})") time.sleep(delay) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 8fb538de..49487c64 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -1,9 +1,11 @@ from datetime import date, datetime +from email.utils import parsedate_to_datetime from io import BytesIO from gzip import GzipFile import logging import json import base64 +import time as _time from dateutil.tz import tzutc from requests import sessions @@ -31,8 +33,16 @@ def parse_retry_after(response): delay = int(retry_after) return min(max(delay, 0), MAX_RETRY_AFTER_SECONDS) except ValueError: - # RFC 7231 allows HTTP-date format (e.g. "Wed, 21 Oct 2015 07:28:00 GMT") - # but we don't parse it; fall back to counted backoff. + pass + + # Try HTTP-date format (RFC 7231 §7.1.1.1) + try: + target_dt = parsedate_to_datetime(retry_after) + delay = int(target_dt.timestamp() - _time.time()) + if delay <= 0: + return None + return min(delay, MAX_RETRY_AFTER_SECONDS) + except (TypeError, ValueError, OverflowError): log = logging.getLogger('segment') log.warning('Unrecognized Retry-After format %r; ignoring header.', retry_after) return None diff --git a/segment/analytics/test/test_client.py b/segment/analytics/test/test_client.py index eb68400c..4d45cf70 100644 --- a/segment/analytics/test/test_client.py +++ b/segment/analytics/test/test_client.py @@ -359,4 +359,28 @@ def mock_post_fn(*args, **kwargs): mock_post.assert_called_once() args, kwargs = mock_post.call_args self.assertIn('proxies', kwargs) - self.assertEqual(kwargs['proxies'], proxies) \ No newline at end of file + self.assertEqual(kwargs['proxies'], proxies) + + def test_queue_full_returns_false(self): + """track() returns (False, msg) when the queue is full — caller should dead-letter""" + client = Client('testsecret', max_queue_size=1) + + # Fill the queue + client.track('user-1', 'First Event') + + # This one should be rejected + success, msg = client.track('user-2', 'Overflow Event') + + self.assertFalse(success) + self.assertEqual(msg['event'], 'Overflow Event') + + def test_queue_full_does_not_raise(self): + """track() never raises when the queue is full — returns False silently""" + client = Client('testsecret', max_queue_size=1) + client.track('user-1', 'First Event') + + try: + success, _ = client.track('user-2', 'Overflow Event') + self.assertFalse(success) + except Exception as e: + self.fail(f'track() raised unexpectedly on full queue: {e}') \ No newline at end of file diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 3419bb0f..22669805 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -384,8 +384,8 @@ def mock_post_fn(*args, **kwargs): self.assertLessEqual(consumer.rate_limited_until, now + 310) self.assertGreater(consumer.rate_limited_until, now + 290) - def test_408_and_503_use_backoff(self): - """Test that 408 and 503 use exponential backoff""" + def test_408_and_503_without_retry_after_use_backoff(self): + """Test that 408 and 503 without Retry-After header use exponential backoff""" track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} for status_code in [408, 503]: @@ -398,7 +398,7 @@ def mock_post_fn(*args, **kwargs): call_count += 1 if call_count == 1: response = mock.Mock() - response.headers = {'Retry-After': '5'} + response.headers = {} # No Retry-After error = APIError(status_code, 'error', 'Error') error.response = response raise error @@ -411,10 +411,54 @@ def mock_sleep(duration): with mock.patch('time.sleep', side_effect=mock_sleep): consumer.request([track]) - # Should use backoff delay (0 for first retry), NOT the Retry-After value of 5 + # Should use backoff delay (0 for first retry), not Retry-After self.assertEqual(call_count, 2) self.assertEqual(len(sleep_durations), 1) - self.assertEqual(sleep_durations[0], 0, f'{status_code} should use backoff, not Retry-After') + self.assertEqual(sleep_durations[0], 0, f'{status_code} without Retry-After should use backoff') + + def test_503_with_retry_after_sets_rate_limit_state(self): + """503 with Retry-After > 0 blocks the pipeline (sets rate_limit_state) and raises""" + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + def mock_post_fn(*args, **kwargs): + response = mock.Mock() + response.headers = {'Retry-After': '2'} + error = APIError(503, 'unavailable', 'Service Unavailable') + error.response = response + raise error + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with self.assertRaises(APIError) as ctx: + consumer.request([track]) + self.assertEqual(ctx.exception.status, 503) + + # Rate-limit state should be set (pipeline-blocking) + self.assertIsNotNone(consumer.rate_limited_until) + self.assertIsNotNone(consumer.rate_limit_start_time) + self.assertGreater(consumer.rate_limited_until, time.time()) + + def test_529_with_retry_after_sets_rate_limit_state(self): + """529 with Retry-After > 0 blocks the pipeline (sets rate_limit_state) and raises""" + consumer = Consumer(None, 'testsecret', retries=2) + track = {'type': 'track', 'event': 'python event', 'userId': 'userId'} + + def mock_post_fn(*args, **kwargs): + response = mock.Mock() + response.headers = {'Retry-After': '3'} + error = APIError(529, 'too_many_requests', 'Too Many Requests') + error.response = response + raise error + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with self.assertRaises(APIError) as ctx: + consumer.request([track]) + self.assertEqual(ctx.exception.status, 529) + + # Rate-limit state should be set (pipeline-blocking) + self.assertIsNotNone(consumer.rate_limited_until) + self.assertIsNotNone(consumer.rate_limit_start_time) + self.assertGreater(consumer.rate_limited_until, time.time()) def test_exponential_backoff_with_jitter(self): """Test that exponential backoff is used for retries without Retry-After""" @@ -982,19 +1026,15 @@ def mock_post_fn(*args, **kwargs): # so it raises on the very first failure (1 attempt total). self.assertEqual(call_count, 1) - def test_parse_retry_after_http_date_logs_warning(self): - """parse_retry_after logs a warning for HTTP-date format and returns None""" - import logging + def test_parse_retry_after_http_date_in_past_returns_none(self): + """parse_retry_after returns None for an HTTP-date in the past""" from segment.analytics.request import parse_retry_after response = mock.Mock() response.headers = {'Retry-After': 'Wed, 21 Oct 2015 07:28:00 GMT'} - with self.assertLogs('segment', level=logging.WARNING) as cm: - result = parse_retry_after(response) - + result = parse_retry_after(response) self.assertIsNone(result) - self.assertTrue(any('Unrecognized Retry-After' in line for line in cm.output)) def test_410_and_460_retried(self): """410 and 460 are retryable status codes""" @@ -1033,3 +1073,29 @@ def mock_post_fn(*args, **kwargs): self.assertEqual(ctx.exception.status, 505) self.assertEqual(call_count, 1) + + def test_retries_exhausted_calls_on_error(self): + """on_error is called with the batch when all retries are exhausted""" + q = Queue() + on_error_calls = [] + + def on_error(error, batch): + on_error_calls.append((error, batch)) + + consumer = Consumer(q, 'testsecret', retries=2, on_error=on_error) + track = {'type': 'track', 'event': 'test event', 'userId': 'user-1'} + q.put(track) + + def mock_post_fn(*args, **kwargs): + raise APIError(500, 'error', 'Server Error') + + with mock.patch('segment.analytics.consumer.post', side_effect=mock_post_fn): + with mock.patch('time.sleep'): + consumer.upload() + + self.assertEqual(len(on_error_calls), 1) + error, batch = on_error_calls[0] + self.assertIsInstance(error, APIError) + self.assertEqual(error.status, 500) + self.assertEqual(len(batch), 1) + self.assertEqual(batch[0]['event'], 'test event') diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 4451dd48..ea54f92b 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -1,4 +1,5 @@ -from datetime import datetime, date +from datetime import datetime, date, timezone, timedelta +import time import unittest import json import requests @@ -188,12 +189,33 @@ def test_parse_retry_after_missing(self): self.assertIsNone(result) def test_parse_retry_after_invalid(self): - """Test parsing with invalid Retry-After header""" + """Test parsing with invalid Retry-After header (garbage string)""" response = mock.Mock() response.headers = {'Retry-After': 'invalid'} result = parse_retry_after(response) self.assertIsNone(result) + def test_parse_retry_after_http_date_future(self): + """Test parsing Retry-After as HTTP-date 2 seconds in future""" + from email.utils import format_datetime + future = datetime.now(tz=timezone.utc) + timedelta(seconds=2) + response = mock.Mock() + response.headers = {'Retry-After': format_datetime(future, usegmt=True)} + result = parse_retry_after(response) + # Should be approximately 2 seconds (allow 1-3 for timing) + self.assertIsNotNone(result) + self.assertGreaterEqual(result, 1) + self.assertLessEqual(result, 3) + + def test_parse_retry_after_http_date_past(self): + """Test parsing Retry-After as HTTP-date in the past returns None""" + from email.utils import format_datetime + past = datetime.now(tz=timezone.utc) - timedelta(seconds=10) + response = mock.Mock() + response.headers = {'Retry-After': format_datetime(past, usegmt=True)} + result = parse_retry_after(response) + self.assertIsNone(result) + def test_oauth_token_cleared_on_511(self): """Test that OAuth token is cleared on 511 status""" oauth_manager = mock.Mock() From e1324dc966d9a8114095133004d4992bf38b66bf Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 16:55:25 -0400 Subject: [PATCH 20/29] Stop stale rate-limit state from misrouting later errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upload() used "self.rate_limited_until is not None" as its test for whether a failure was a rate limit, but that field was only cleared on success or once max_rate_limit_duration elapsed — never when its wait simply passed. After any rate-limit episode, every subsequent APIError, including permanently non-retryable ones, therefore took the re-queue branch instead of the drop branch. A 429 with Retry-After followed by a steady 400 re-queued the same batch forever: no sleep, because the deadline was already in the past, so roughly 16 uploads a second, on_error never called, and queue.join() — and so flush(), shutdown() and the atexit hook — blocked until max_rate_limit_duration, 12 hours by default. request() now marks the exception it raises from the rate-limit path, and upload() classifies on that instead of on consumer state. The episode gate is rate_limit_start_time, and rate_limited_until is cleared once its wait has been served, so a spent deadline can no longer classify anything. Two further fixes: - The Retry-After wait was a single uninterruptible sleep of up to MAX_RETRY_AFTER_SECONDS (300s), so pause(), join() and the atexit hook blocked for its full duration. It now waits in one-second slices and checks self.running, matching the bounded-shutdown behaviour of the other SDKs. - parse_retry_after read a naive HTTP-date in the host's local timezone. parsedate_to_datetime returns a naive datetime for the RFC 5322 "-0000" offset that servers do emit, so on a UTC+14 host a date 120s in the future parsed as 14 hours in the past and returned None — silently discarding the server's instruction and falling through to backoff. Naive datetimes are now read as UTC. 131 unit tests and all 58 e2e tests pass. --- segment/analytics/consumer.py | 65 ++++++++++++++++++------- segment/analytics/request.py | 8 ++- segment/analytics/test/test_consumer.py | 41 ++++++++++++++++ 3 files changed, 95 insertions(+), 19 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 14a7d567..cf09c0ef 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -87,6 +87,30 @@ def pause(self): """Pause the consumer.""" self.running = False + def _wait(self, seconds): + """Sleep in slices so pause()/join()/atexit are not blocked for up to + MAX_RETRY_AFTER_SECONDS. Returns False if the consumer was stopped.""" + deadline = time.time() + seconds + while self.running: + remaining = deadline - time.time() + if remaining <= 0: + return True + time.sleep(min(1.0, remaining)) + return False + + def _requeue(self, batch): + """Put a batch back on the queue, reporting anything that no longer fits.""" + dropped = [] + for item in batch: + try: + self.queue.put(item, block=False) + except Exception: + dropped.append(item) + if dropped: + self.log.error("Queue full during rate-limit re-queue. Dropping %d item(s).", len(dropped)) + if self.on_error: + self.on_error(Exception("Queue full, items dropped during rate-limit re-queue"), dropped) + def set_rate_limit_state(self, response): """Set rate-limit state from a 429 response with a valid Retry-After header.""" retry_after = parse_retry_after(response) if response is not None else None @@ -107,12 +131,15 @@ def upload(self): if len(batch) == 0: return False - # Check rate-limit state before attempting upload - if self.rate_limited_until is not None: + # Check rate-limit state before attempting upload. Gate on the episode + # marker, not on rate_limited_until: the latter is cleared as soon as its + # wait has been served, so it cannot be used to decide whether we are + # still inside a rate-limit episode. + if self.rate_limit_start_time is not None: now = time.time() # Check if maxRateLimitDuration has been exceeded - if self.rate_limit_start_time is not None and now - self.rate_limit_start_time > self.max_rate_limit_duration: + if now - self.rate_limit_start_time > self.max_rate_limit_duration: self.log.error( "Rate limit duration exceeded (%ds). Clearing rate-limit state and dropping batch.", self.max_rate_limit_duration ) @@ -125,10 +152,18 @@ def upload(self): return False # Still rate-limited; wait until the rate limit expires - wait_time = self.rate_limited_until - now - if wait_time > 0: - self.log.debug("Rate-limited. Waiting %.2fs before next upload attempt.", wait_time) - time.sleep(wait_time) + if self.rate_limited_until is not None: + wait_time = self.rate_limited_until - now + if wait_time > 0: + self.log.debug("Rate-limited. Waiting %.2fs before next upload attempt.", wait_time) + if not self._wait(wait_time): + # Shutting down: leave the batch queued rather than + # uploading into a consumer that is stopping. + self._requeue(batch) + return False + # The wait has been served. Clearing it here keeps a stale + # timestamp from classifying later, unrelated errors as rate limits. + self.rate_limited_until = None try: self.request(batch) @@ -136,18 +171,9 @@ def upload(self): self.clear_rate_limit_state() success = True except APIError as e: - if self.rate_limited_until is not None: + if getattr(e, "rate_limited", False): self.log.debug("Rate-limited (status %d). Re-queuing batch and halting upload iteration.", e.status) - dropped = [] - for item in batch: - try: - self.queue.put(item, block=False) - except Exception: - dropped.append(item) - if dropped: - self.log.error("Queue full during rate-limit re-queue. Dropping %d item(s).", len(dropped)) - if self.on_error: - self.on_error(Exception("Queue full, items dropped during rate-limit re-queue"), dropped) + self._requeue(batch) success = False else: self.log.error("error uploading: %s", e) @@ -280,6 +306,9 @@ def apply_backoff(e, label): retry_after = parse_retry_after(e.response) if e.response is not None else None if retry_after is not None and retry_after > 0: self.set_rate_limit_state(e.response) + # Tell upload() this specific failure is a rate limit. Inferring + # it from consumer state misclassifies every later error. + e.rate_limited = True raise # No Retry-After: counted backoff diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 5f055f95..8ab61670 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -2,7 +2,7 @@ import json import logging import time as _time -from datetime import date, datetime +from datetime import date, datetime, timezone from email.utils import parsedate_to_datetime from gzip import GzipFile from io import BytesIO @@ -38,6 +38,12 @@ def parse_retry_after(response): # Try HTTP-date format (RFC 7231 §7.1.1.1) try: target_dt = parsedate_to_datetime(retry_after) + if target_dt.tzinfo is None: + # parsedate_to_datetime returns a naive datetime for the RFC 5322 + # "-0000" offset, which servers do emit. timestamp() would then read + # it in the host's local zone, so the same header yields different + # delays — or None — depending on where the process runs. + target_dt = target_dt.replace(tzinfo=timezone.utc) delay = int(target_dt.timestamp() - _time.time()) if delay <= 0: return None diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index dff51a65..c7115dcc 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -1,4 +1,5 @@ import json +import threading import time import unittest @@ -420,6 +421,46 @@ def mock_post_fn(*args, **kwargs): self.assertIsNotNone(consumer.rate_limit_start_time) self.assertGreater(consumer.rate_limited_until, time.time()) + def test_stale_rate_limit_state_does_not_misroute_later_errors(self): + """A past rate-limit episode must not make later non-retryable errors look rate-limited""" + q = Queue() + consumer = Consumer(q, "testsecret", retries=1) + consumer.on_error = mock.Mock() + track = {"type": "track", "event": "python event", "userId": "userId"} + q.put(track) + + # Simulate having been rate-limited a moment ago and already served the wait. + consumer.rate_limit_start_time = time.time() - 1 + consumer.rate_limited_until = time.time() - 0.5 + + def mock_post_fn(*args, **kwargs): + raise APIError(400, "bad_request", "Bad Request") + + with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): + consumer.upload() + + # The 400 is non-retryable: it must be dropped and reported, not re-queued. + self.assertTrue(consumer.on_error.called) + self.assertEqual(q.qsize(), 0) + + def test_rate_limit_wait_is_interruptible(self): + """pause() must break the Retry-After wait rather than blocking for its full duration""" + consumer = Consumer(Queue(), "testsecret") + + def stop_soon(): + time.sleep(0.2) + consumer.pause() + + t = threading.Thread(target=stop_soon) + t.start() + started = time.time() + completed = consumer._wait(30) + elapsed = time.time() - started + t.join() + + self.assertFalse(completed, "the wait should report that it was interrupted") + self.assertLess(elapsed, 5, f"pause() did not interrupt the wait; it took {elapsed:.1f}s") + def test_exponential_backoff_with_jitter(self): """Test that exponential backoff is used for retries without Retry-After""" consumer = Consumer(None, "testsecret", retries=4) From ff63200216012337b659e91f025b56c306118f89 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 11 Sep 2026 11:11:09 -0400 Subject: [PATCH 21/29] Tighten retry comments Cut the before/after narration from the comments added with the Retry-After work; the diff carries that. What is left states the invariant a maintainer needs: that rate_limit_start_time marks the episode while rate_limited_until is only the current deadline, and that upload() classifies on the flag rather than consumer state. --- segment/analytics/consumer.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index cf09c0ef..82f8b6f6 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -131,10 +131,8 @@ def upload(self): if len(batch) == 0: return False - # Check rate-limit state before attempting upload. Gate on the episode - # marker, not on rate_limited_until: the latter is cleared as soon as its - # wait has been served, so it cannot be used to decide whether we are - # still inside a rate-limit episode. + # rate_limit_start_time marks the episode; rate_limited_until is only the + # current deadline and is cleared once served, so gate on the former. if self.rate_limit_start_time is not None: now = time.time() @@ -161,8 +159,7 @@ def upload(self): # uploading into a consumer that is stopping. self._requeue(batch) return False - # The wait has been served. Clearing it here keeps a stale - # timestamp from classifying later, unrelated errors as rate limits. + # Clear the served deadline so it cannot classify a later error. self.rate_limited_until = None try: @@ -306,8 +303,8 @@ def apply_backoff(e, label): retry_after = parse_retry_after(e.response) if e.response is not None else None if retry_after is not None and retry_after > 0: self.set_rate_limit_state(e.response) - # Tell upload() this specific failure is a rate limit. Inferring - # it from consumer state misclassifies every later error. + # upload() classifies on this flag rather than consumer state, + # which may still hold an earlier episode's deadline. e.rate_limited = True raise From 4242e0527e218e236b75b56e95677f66a12bb83d Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 17 Sep 2026 15:21:58 -0400 Subject: [PATCH 22/29] Opt in to the e2e Authorization check The header assertion in sdk-e2e-tests is opt-in per SDK, since analytics-kotlin and analytics-swift do not send it yet. This SDK does, so it runs the check. --- e2e-cli/e2e-config.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index e1a02d5b..a1596448 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -3,5 +3,7 @@ "test_suites": "basic,retry", "auto_settings": false, "patch": null, - "env": {} + "env": { + "AUTH_HEADER": "true" + } } From 0f0c680df85fa979cfe39903e465848c1a12c0e8 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 12:04:13 -0400 Subject: [PATCH 23/29] Treat only 2xx as a successful upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every one of these SDKs treated a 3xx as a failure before this work, and the change to 200-399 came from the design doc's "Spec item 1: 2xx and 3xx are success". That line is wrong, and the doc is what needs correcting. Measured against a local server, with the same HTTP clients these SDKs use: 307/308 + Location -> followed as POST with the body, arrives as 200 301/302/303 + Loc. -> followed as GET with no body, arrives as 200 302 without Location-> surfaces raw as 302 300 Multiple Choices-> surfaces raw as 300 304 Not Modified -> surfaces raw as 304 So a raw 3xx only reaches the classifier when the client has already declined to follow it, meaning nothing was uploaded. The one redirect that genuinely works, 307/308, never produces a 3xx here at all — it produces 200 — so narrowing the bound cannot break it. Nothing was gained by the wider range; a 300, 304, or Location-less 302 from a proxy was being logged as a delivered batch and dropped with no error callback. The narrower bound also needs no new branches: a 3xx is neither 5xx nor in the retryable 4xx set, so it already falls through to the non-retryable path and reports a failure. TAPI does not emit 3xx and has no plans to. This matters because host is customer-configurable and proxies in front of it are common. requests declines the redirect, so this replaces a misleading "Unknown error: [302]" with a named redirect failure. Tests split: 2xx is success, 3xx raises APIError with code "redirect". --- segment/analytics/request.py | 13 ++++++++++++- segment/analytics/test/test_request.py | 20 ++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 8ab61670..f0568a1b 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -106,10 +106,21 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag except Exception as e: raise e - if 200 <= res.status_code < 400: + if 200 <= res.status_code < 300: log.debug("data uploaded successfully") return res + if 300 <= res.status_code < 400: + # requests follows any redirect it can, so a 3xx arriving here means it + # declined to: no Location, a 300, or a 304. Nothing was uploaded, and + # reporting it as "unknown" below would hide a misconfigured host. + log.error( + "Unexpected redirect (%s) from %s; batch not uploaded. Check whether the configured host points at a proxy or redirector.", + res.status_code, + url, + ) + raise APIError(res.status_code, "redirect", res.reason, res) + if oauth_manager and res.status_code in [400, 401, 403, 511]: oauth_manager.clear_token() diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 6f2b350a..066c3c49 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -120,8 +120,8 @@ def mock_post_fn(*args, **kwargs): self.assertEqual(headers["X-Retry-Count"], "5") def test_non_200_2xx_treated_as_success(self): - """Test that 2xx and 3xx status codes are treated as success""" - for status_code in [200, 201, 204, 301, 302]: + """Test that all 2xx status codes are treated as success, not just 200""" + for status_code in [200, 201, 202, 204]: def mock_post_fn(*args, **kwargs): res = mock.Mock() @@ -132,6 +132,22 @@ def mock_post_fn(*args, **kwargs): res = post("testsecret", batch=[{"userId": "userId", "event": "python event", "type": "track"}]) self.assertEqual(res.status_code, status_code) + def test_3xx_is_not_success(self): + """requests follows what it can, so a 3xx here means nothing was uploaded""" + for status_code in [300, 301, 302, 304]: + + def mock_post_fn(*args, **kwargs): + res = mock.Mock() + res.status_code = status_code + res.reason = "Redirect" + return res + + with mock.patch("segment.analytics.request._session.post", side_effect=mock_post_fn): + with self.assertRaises(APIError) as ctx: + post("testsecret", batch=[{"userId": "userId", "event": "python event", "type": "track"}]) + self.assertEqual(ctx.exception.status, status_code) + self.assertEqual(ctx.exception.code, "redirect") + def test_parse_retry_after_integer(self): """Test parsing Retry-After header with integer seconds""" response = mock.Mock() From c8a18f427b6f7a8a6adf9ae9379f502ced398b49 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:40:32 -0400 Subject: [PATCH 24/29] Fix a task_done leak, finish interruptible waits, use a monotonic clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from review. The shutdown path added with the interruptible Retry-After wait returned before the try/finally that discharges queue.task_done(), so every item taken by queue.get() for that batch left an outstanding obligation and queue.join() — and therefore flush(), shutdown() and the atexit hook — never completed. The batch is still handed back; the get() obligations are now discharged alongside, and the re-queued copies carry their own. The backoff waits were still bare time.sleep() calls, so only the rate-limit wait honoured pause(). Both now go through _wait and re-raise if the consumer stopped, letting upload() drop the batch through its normal error path. Four tests recorded time.sleep to assert the backoff schedule; because _wait sleeps in slices they now record at the _wait boundary, which is the delay they were really asserting. Durations were measured with time.time(), so a clock adjustment could expire or extend the rate-limit and backoff budgets. All seven duration sites in consumer.py use time.monotonic(). request.py keeps wall-clock time, since it is comparing against an absolute HTTP-date. 132 tests and all 61 e2e tests pass. --- segment/analytics/consumer.py | 29 +++++++++++++-------- segment/analytics/test/test_consumer.py | 34 ++++++++++++------------- 2 files changed, 35 insertions(+), 28 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 82f8b6f6..7f5bbe49 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -90,9 +90,9 @@ def pause(self): def _wait(self, seconds): """Sleep in slices so pause()/join()/atexit are not blocked for up to MAX_RETRY_AFTER_SECONDS. Returns False if the consumer was stopped.""" - deadline = time.time() + seconds + deadline = time.monotonic() + seconds while self.running: - remaining = deadline - time.time() + remaining = deadline - time.monotonic() if remaining <= 0: return True time.sleep(min(1.0, remaining)) @@ -115,9 +115,9 @@ def set_rate_limit_state(self, response): """Set rate-limit state from a 429 response with a valid Retry-After header.""" retry_after = parse_retry_after(response) if response is not None else None if retry_after is not None: - self.rate_limited_until = time.time() + retry_after + self.rate_limited_until = time.monotonic() + retry_after if self.rate_limit_start_time is None: - self.rate_limit_start_time = time.time() + self.rate_limit_start_time = time.monotonic() def clear_rate_limit_state(self): """Clear rate-limit state after successful request or duration exceeded.""" @@ -134,7 +134,7 @@ def upload(self): # rate_limit_start_time marks the episode; rate_limited_until is only the # current deadline and is cleared once served, so gate on the former. if self.rate_limit_start_time is not None: - now = time.time() + now = time.monotonic() # Check if maxRateLimitDuration has been exceeded if now - self.rate_limit_start_time > self.max_rate_limit_duration: @@ -155,9 +155,14 @@ def upload(self): if wait_time > 0: self.log.debug("Rate-limited. Waiting %.2fs before next upload attempt.", wait_time) if not self._wait(wait_time): - # Shutting down: leave the batch queued rather than - # uploading into a consumer that is stopping. + # Shutting down: hand the batch back rather than uploading + # into a consumer that is stopping. This returns before the + # try/finally below, so the get() obligations have to be + # discharged here or queue.join() never completes; the + # re-queued copies carry their own fresh obligations. self._requeue(batch) + for _ in batch: + self.queue.task_done() return False # Clear the served deadline so it cannot classify a later error. self.rate_limited_until = None @@ -253,8 +258,8 @@ def apply_backoff(e, label): """Apply retry backoff logic. Returns delay if should retry, raises if exhausted.""" nonlocal first_failure_time, backoff_attempts if first_failure_time is None: - first_failure_time = time.time() - if time.time() - first_failure_time >= self.max_total_backoff_duration: + first_failure_time = time.monotonic() + if time.monotonic() - first_failure_time >= self.max_total_backoff_duration: self.log.error( f"Max total backoff duration ({self.max_total_backoff_duration}s) exceeded " f"after {total_attempts} attempts. Final error: {e}" @@ -310,8 +315,10 @@ def apply_backoff(e, label): # No Retry-After: counted backoff delay = apply_backoff(e, f"Retry attempt (status {e.status})") - time.sleep(delay) + if not self._wait(delay): + raise except Exception as e: delay = apply_backoff(e, "Network error retry") - time.sleep(delay) + if not self._wait(delay): + raise diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index c7115dcc..0dd7447d 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -321,7 +321,7 @@ def mock_post_fn(*args, **kwargs): self.assertIsNotNone(consumer.rate_limited_until) self.assertIsNotNone(consumer.rate_limit_start_time) # rate_limited_until should be ~10 seconds in the future - self.assertGreater(consumer.rate_limited_until, time.time() + 5) + self.assertGreater(consumer.rate_limited_until, time.monotonic() + 5) def test_retry_after_capped_at_300_seconds(self): """Test that Retry-After delay is capped at 300 seconds when setting rate-limit state""" @@ -335,7 +335,7 @@ def mock_post_fn(*args, **kwargs): error.response = response raise error - now = time.time() + now = time.monotonic() with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): with self.assertRaises(APIError): consumer.request([track]) @@ -369,7 +369,7 @@ def mock_sleep(duration): sleep_durations.append(duration) with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): - with mock.patch("time.sleep", side_effect=mock_sleep): + with mock.patch.object(Consumer, "_wait", side_effect=lambda seconds: mock_sleep(seconds) or True): consumer.request([track]) # Should use backoff delay (0 for first retry), not Retry-After @@ -397,7 +397,7 @@ def mock_post_fn(*args, **kwargs): # Rate-limit state should be set (pipeline-blocking) self.assertIsNotNone(consumer.rate_limited_until) self.assertIsNotNone(consumer.rate_limit_start_time) - self.assertGreater(consumer.rate_limited_until, time.time()) + self.assertGreater(consumer.rate_limited_until, time.monotonic()) def test_529_with_retry_after_sets_rate_limit_state(self): """529 with Retry-After > 0 blocks the pipeline (sets rate_limit_state) and raises""" @@ -419,7 +419,7 @@ def mock_post_fn(*args, **kwargs): # Rate-limit state should be set (pipeline-blocking) self.assertIsNotNone(consumer.rate_limited_until) self.assertIsNotNone(consumer.rate_limit_start_time) - self.assertGreater(consumer.rate_limited_until, time.time()) + self.assertGreater(consumer.rate_limited_until, time.monotonic()) def test_stale_rate_limit_state_does_not_misroute_later_errors(self): """A past rate-limit episode must not make later non-retryable errors look rate-limited""" @@ -430,8 +430,8 @@ def test_stale_rate_limit_state_does_not_misroute_later_errors(self): q.put(track) # Simulate having been rate-limited a moment ago and already served the wait. - consumer.rate_limit_start_time = time.time() - 1 - consumer.rate_limited_until = time.time() - 0.5 + consumer.rate_limit_start_time = time.monotonic() - 1 + consumer.rate_limited_until = time.monotonic() - 0.5 def mock_post_fn(*args, **kwargs): raise APIError(400, "bad_request", "Bad Request") @@ -453,9 +453,9 @@ def stop_soon(): t = threading.Thread(target=stop_soon) t.start() - started = time.time() + started = time.monotonic() completed = consumer._wait(30) - elapsed = time.time() - started + elapsed = time.monotonic() - started t.join() self.assertFalse(completed, "the wait should report that it was interrupted") @@ -482,7 +482,7 @@ def mock_sleep(duration): sleep_durations.append(duration) with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): - with mock.patch("time.sleep", side_effect=mock_sleep): + with mock.patch.object(Consumer, "_wait", side_effect=lambda seconds: mock_sleep(seconds) or True): consumer.request([track]) # Should have 3 backoff delays @@ -610,7 +610,7 @@ def mock_sleep(duration): sleep_duration = duration with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): - with mock.patch("time.sleep", side_effect=mock_sleep): + with mock.patch.object(Consumer, "_wait", side_effect=lambda seconds: mock_sleep(seconds) or True): consumer.request([track]) # Should have two attempts @@ -647,7 +647,7 @@ def mock_sleep(duration): sleep_duration = duration with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): - with mock.patch("time.sleep", side_effect=mock_sleep): + with mock.patch.object(Consumer, "_wait", side_effect=lambda seconds: mock_sleep(seconds) or True): consumer.request([track]) # Should have two attempts @@ -864,7 +864,7 @@ def mock_time(): with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): with mock.patch("time.sleep"): - with mock.patch("time.time", side_effect=mock_time): + with mock.patch("time.monotonic", side_effect=mock_time): with self.assertRaises(APIError) as ctx: consumer.request([track]) self.assertEqual(ctx.exception.status, 500) @@ -882,7 +882,7 @@ def test_t20_max_rate_limit_duration(self): track = {"type": "track", "event": "python event", "userId": "userId"} # Pre-set rate-limit state as if we entered it 15 seconds ago - now = time.time() + now = time.monotonic() consumer.rate_limit_start_time = now - 15 # 15s ago, exceeds 10s limit consumer.rate_limited_until = now + 5 # Would still be rate-limited @@ -913,8 +913,8 @@ def test_rate_limit_state_cleared_on_success(self): track = {"type": "track", "event": "python event", "userId": "userId"} # Set rate-limit state - consumer.rate_limited_until = time.time() - 1 # Already expired - consumer.rate_limit_start_time = time.time() - 10 + consumer.rate_limited_until = time.monotonic() - 1 # Already expired + consumer.rate_limit_start_time = time.monotonic() - 10 q.put(track) @@ -1021,7 +1021,7 @@ def mock_post_fn(*args, **kwargs): consumer.request([track]) # With duration=0 and >= check, first failure sets first_failure_time - # and immediately satisfies time.time() - first_failure_time >= 0, + # and immediately satisfies time.monotonic() - first_failure_time >= 0, # so it raises on the very first failure (1 attempt total). self.assertEqual(call_count, 1) From ba013d21b20fea1cea7887c380b3be23551952f0 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:57:08 -0400 Subject: [PATCH 25/29] Add release notes for the HTTP response and retry work Records the retry/Retry-After work and, for the SDKs where a header is newly on the wire, an upgrade note: customers whose proxies allowlist request headers had uploads rejected by the already-released analytics-next change, and the same trap applies here. --- HISTORY.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/HISTORY.md b/HISTORY.md index f0fb99e1..5f3a641d 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,3 +1,22 @@ +# Unreleased +### Upgrade note: new request headers and proxy allowlists +This release sends two request headers that earlier versions did not: +`Authorization` (HTTP Basic, carrying your write key) and `X-Retry-Count` +(on retries only). If your traffic to Segment goes through a proxy, gateway +or WAF that allowlists request headers, add both before upgrading or uploads +will be rejected. + +- Send the write key as an `Authorization: Basic` header. It is still included in the request body, so no server-side change is required. OAuth deployments continue to send `Authorization: Bearer` and are unaffected. +- Send `X-Retry-Count` on retries, so the server can distinguish a retry from a first attempt. Omitted on the first attempt. +- Unified retry handling: 429, 408, 410, 460 and 5xx (except 501, 505 and 511) are retried. `Retry-After` is honoured on all of them, not just 429, which brings 529 in through the generic 5xx rule. +- `Retry-After` accepts numeric seconds and the RFC 7231 HTTP-date formats, capped at 300s. +- Rate-limited retries are bounded by elapsed time rather than counted against the retry limit, so a long `Retry-After` no longer exhausts the budget. +- New client options `max_total_backoff_duration` and `max_rate_limit_duration` (default 12 hours each) bound the two waits. +- Only 2xx responses count as a successful upload. A 3xx is now logged and retried rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom `host` values. +- Backoff waits are interruptible, so `shutdown()` no longer blocks for the full delay. +- Retry timing uses a monotonic clock, so a system clock change cannot stretch or collapse a backoff. +- Fix a `queue.task_done()` leak that could leave `flush()` waiting forever when a batch was re-queued during shutdown. + # 2.3.6 / 2026-4-7 - Update and widen PyJWT version to address security issue From 2b1eae401845117267d39900854c97c83d1fd1d2 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 07:06:17 -0400 Subject: [PATCH 26/29] Correct the release notes on 3xx handling No SDK retries a 3xx: every one classifies it as non-retryable and reports a failed upload. The notes claimed it was retried, which is wrong, and would have sent anyone debugging a proxy redirect looking for retries that never happen. Also scopes python's 511 line to the OAuth case, which is the one place the spec does allow a 511 retry, and php's new budget options to the LibCurl consumer, since Socket ignores them. --- HISTORY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 5f3a641d..d858772e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,11 +8,11 @@ will be rejected. - Send the write key as an `Authorization: Basic` header. It is still included in the request body, so no server-side change is required. OAuth deployments continue to send `Authorization: Bearer` and are unaffected. - Send `X-Retry-Count` on retries, so the server can distinguish a retry from a first attempt. Omitted on the first attempt. -- Unified retry handling: 429, 408, 410, 460 and 5xx (except 501, 505 and 511) are retried. `Retry-After` is honoured on all of them, not just 429, which brings 529 in through the generic 5xx rule. +- Unified retry handling: 429, 408, 410, 460 and 5xx (except 501 and 505) are retried. `Retry-After` is honoured on all of them, not just 429, which brings 529 in through the generic 5xx rule. 511 asks the client to re-authenticate, so it is retried only when an `oauth_manager` is configured and is dropped otherwise. - `Retry-After` accepts numeric seconds and the RFC 7231 HTTP-date formats, capped at 300s. - Rate-limited retries are bounded by elapsed time rather than counted against the retry limit, so a long `Retry-After` no longer exhausts the budget. - New client options `max_total_backoff_duration` and `max_rate_limit_duration` (default 12 hours each) bound the two waits. -- Only 2xx responses count as a successful upload. A 3xx is now logged and retried rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom `host` values. +- Only 2xx responses count as a successful upload. A 3xx is now reported as a failed upload rather than silently treated as delivered. It is not retried: a redirect the HTTP client already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom `host` values. - Backoff waits are interruptible, so `shutdown()` no longer blocks for the full delay. - Retry timing uses a monotonic clock, so a system clock change cannot stretch or collapse a backoff. - Fix a `queue.task_done()` leak that could leave `flush()` waiting forever when a batch was re-queued during shutdown. From 0af186e04aa786e9836150042db023a9a64fa2f5 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 09:01:02 -0400 Subject: [PATCH 27/29] Re-queue a batch interrupted mid-backoff, and parse Retry-After once Shutting down during a counted-backoff wait dropped the batch and reported it through on_error, while shutting down during a Retry-After wait handed it back. The asymmetry favoured the rare case: the counted path is what a 500, a timeout or a 429 without Retry-After takes. A batch interrupted there has not exhausted its retry budget or its duration budget, so the wait was interrupted, not the upload failed. ShutdownInterrupted separates the two so upload() can tell them apart. The new test fails without the fix (queue empty, on_error fired) rather than passing either way. set_rate_limit_state now takes the parsed delay instead of re-deriving it from the response. Parsing an HTTP-date Retry-After reads the wall clock and truncates to whole seconds, so two parses of one response can straddle a second boundary: the first sees 1 and opens the episode, the second sees 0 and leaves rate_limited_until unset. The next attempt then found an open episode with no deadline and skipped its wait entirely. Its docstring also still said "from a 429 response", from before Retry-After was honoured on every retryable status. 133 unit tests and the full 61-test e2e suite pass; ruff check and format are clean. --- segment/analytics/consumer.py | 35 ++++++++++++++++++++----- segment/analytics/test/test_consumer.py | 33 +++++++++++++++++++++++ 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 7f5bbe49..deec68be 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -7,6 +7,15 @@ from segment.analytics.request import APIError, DatetimeSerializer, parse_retry_after, post + +class ShutdownInterrupted(Exception): + """A retry wait was cut short by shutdown. + + Distinct from an upload failure: the batch has not exhausted its budget, so it + is re-queued rather than reported through on_error. + """ + + MAX_MSG_SIZE = 32 << 10 # Our servers only accept batches less than 500KB. Here limit is set slightly @@ -111,9 +120,15 @@ def _requeue(self, batch): if self.on_error: self.on_error(Exception("Queue full, items dropped during rate-limit re-queue"), dropped) - def set_rate_limit_state(self, response): - """Set rate-limit state from a 429 response with a valid Retry-After header.""" - retry_after = parse_retry_after(response) if response is not None else None + def set_rate_limit_state(self, retry_after): + """Open or extend a rate-limit episode using an already-parsed Retry-After. + + Takes the delay rather than the response on purpose. Parsing an HTTP-date + Retry-After reads the wall clock and truncates to whole seconds, so parsing + twice for one response can straddle a second boundary and disagree with + itself — leaving the episode open with no deadline, which skipped the wait + entirely on the next attempt. + """ if retry_after is not None: self.rate_limited_until = time.monotonic() + retry_after if self.rate_limit_start_time is None: @@ -172,6 +187,14 @@ def upload(self): # Success — clear rate-limit state self.clear_rate_limit_state() success = True + except ShutdownInterrupted: + # Budget was not exhausted; the wait was. Hand the batch back so the + # next run uploads it, rather than reporting a failure that did not + # happen. Matches the rate-limited wait above. + self.log.debug("Shutting down during retry backoff. Re-queuing batch.") + self._requeue(batch) + success = False + except APIError as e: if getattr(e, "rate_limited", False): self.log.debug("Rate-limited (status %d). Re-queuing batch and halting upload iteration.", e.status) @@ -307,7 +330,7 @@ def apply_backoff(e, label): # Any retryable status with valid Retry-After > 0: block pipeline, re-queue retry_after = parse_retry_after(e.response) if e.response is not None else None if retry_after is not None and retry_after > 0: - self.set_rate_limit_state(e.response) + self.set_rate_limit_state(retry_after) # upload() classifies on this flag rather than consumer state, # which may still hold an earlier episode's deadline. e.rate_limited = True @@ -316,9 +339,9 @@ def apply_backoff(e, label): # No Retry-After: counted backoff delay = apply_backoff(e, f"Retry attempt (status {e.status})") if not self._wait(delay): - raise + raise ShutdownInterrupted() from e except Exception as e: delay = apply_backoff(e, "Network error retry") if not self._wait(delay): - raise + raise ShutdownInterrupted() from e diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 0dd7447d..5bbc58b1 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -461,6 +461,39 @@ def stop_soon(): self.assertFalse(completed, "the wait should report that it was interrupted") self.assertLess(elapsed, 5, f"pause() did not interrupt the wait; it took {elapsed:.1f}s") + def test_shutdown_during_backoff_requeues_instead_of_dropping(self): + """A batch interrupted mid-backoff still has budget, so it must be handed back. + + The rate-limited wait already re-queued on shutdown; the counted-backoff wait + reported a hard failure and dropped the batch instead, which is the common case + (a 500 or a timeout, not a Retry-After). + """ + q = Queue() + consumer = Consumer(q, "testsecret", retries=10) + track = {"type": "track", "event": "python event", "userId": "userId"} + q.put(track) + + errors = [] + consumer.on_error = lambda e, batch: errors.append(e) + + # Fail with a retryable status carrying no Retry-After, so the batch takes + # the counted-backoff path, then shut down while it waits. + def mock_post_fn(*args, **kwargs): + raise APIError(503, "service_unavailable", "Service Unavailable") + + def stop_soon(): + time.sleep(0.2) + consumer.pause() + + t = threading.Thread(target=stop_soon) + t.start() + with mock.patch("segment.analytics.consumer.post", side_effect=mock_post_fn): + consumer.upload() + t.join() + + self.assertEqual(q.qsize(), 1, "the interrupted batch should have been re-queued") + self.assertEqual(errors, [], "shutdown is not an upload failure; on_error should not fire") + def test_exponential_backoff_with_jitter(self): """Test that exponential backoff is used for retries without Retry-After""" consumer = Consumer(None, "testsecret", retries=4) From 021932375dd34342a65e3982672be5e31d06437e Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:16:40 -0400 Subject: [PATCH 28/29] Keep a non-object JSON error body non-retryable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit post() reads the error body as payload["code"], which raises TypeError — not KeyError — when the body is valid JSON but not an object. A list, string, number or null all subscript that way, so the exception escaped the handler, surfaced from post() as a generic error, and the consumer's network-error branch retried it. A non-retryable 4xx was therefore retried ten times whenever the body was not a JSON object. The new test covers all four shapes and fails without this change with "TypeError: list indices must be integers or slices, not str". 134 unit tests, ruff clean, 61-test e2e suite passes. --- segment/analytics/request.py | 6 +++++- segment/analytics/test/test_request.py | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/segment/analytics/request.py b/segment/analytics/request.py index f0568a1b..8f3ee0e0 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -128,7 +128,11 @@ def post(write_key, host=None, gzip=False, timeout=15, proxies=None, oauth_manag payload = res.json() log.debug("received response: %s", payload) raise APIError(res.status_code, payload["code"], payload["message"], res) - except (ValueError, KeyError): + except (ValueError, KeyError, TypeError): + # TypeError covers a body that is valid JSON but not an object: a list, + # string or number subscripts with TypeError rather than KeyError. Without + # it that escaped as a generic exception and the consumer retried a + # non-retryable 4xx as though the network had failed. log.error("Unknown error: [%s] %s", res.status_code, res.reason) raise APIError(res.status_code, "unknown", res.text, res) diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 066c3c49..2ac5f65e 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -234,3 +234,28 @@ def mock_post_fn(*args, **kwargs): self.assertIsNotNone(e.response) else: self.fail("Expected APIError to be raised") + + +class TestNonObjectErrorBody(unittest.TestCase): + def test_non_object_json_body_raises_apierror_not_typeerror(self): + """A 400 whose body is valid JSON but not an object must stay non-retryable. + + payload["code"] subscripts a list, string or number with TypeError rather + than KeyError, so without TypeError in the handler it escaped as a generic + exception and the consumer retried a non-retryable 4xx ten times. + """ + for body in ("[]", '"oops"', "5", "null"): + with self.subTest(body=body): + res = mock.Mock() + res.status_code = 400 + res.json.return_value = json.loads(body) + res.text = body + res.reason = "Bad Request" + + with mock.patch("segment.analytics.request._session") as session: + session.post.return_value = res + with self.assertRaises(APIError) as ctx: + post("write_key", batch=[{"userId": "u", "type": "track"}]) + + self.assertEqual(400, ctx.exception.status) + self.assertEqual("unknown", ctx.exception.code) From 3039c4f9b3be8ff3c91e9a6685f5fcd27d4eabf6 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:46:30 -0400 Subject: [PATCH 29/29] Revert unrelated README quote-style churn Two examples had their quotes changed from single to double. Nothing to do with retry handling, and markdown is not something ruff formats, so it was a stray edit. Removing it so the diff a reviewer reads is only the change it claims to be. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f159b7ba..fdcd72cf 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ Analytics helps you measure your users, product, and business. It unlocks insigh For example, you can capture data on any app: ```python - analytics.track("Order Completed", {price: 99.84}) + analytics.track('Order Completed', { price: 99.84 }) ``` Then, query the resulting data in SQL: ```sql @@ -71,7 +71,7 @@ Now inside your app, you'll want to **set your** `write_key` before making any a ```python import segment.analytics as analytics -analytics.write_key = "YOUR_WRITE_KEY" +analytics.write_key = 'YOUR_WRITE_KEY' ``` **Note** If you need to send data to multiple Segment sources, you can initialize a new Client for each `write_key`