From 46d9f10bfeb6a6876fa0e7f0eebc76af9b7c47b6 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 16:38:21 -0400 Subject: [PATCH 01/10] Bound the rate-limit budget at 5 minutes instead of 12 hours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 12 hour default was designed as a last-ditch backstop, on the assumption that a retry count would stop us ever reaching it. Nothing counts rate-limited attempts — that exemption is deliberate, so a compliant server-directed wait does not burn the error budget — which left the duration as the only limit rather than the backstop. A server that kept sending Retry-After could hold a batch for half a day, and with the default single consumer thread that stalls all delivery and blocks flush() and shutdown() for the same period. Five minutes matches the counted path's ~4 minute worst case, so the two failure modes now cost about the same. Retry-After is capped at 60s rather than 300s. At 300s the cap equalled the whole budget, so a single sleep consumed it and the rate-limit path degenerated to one attempt. 60s buys roughly five. Segment serving a longer Retry-After would mean something has gone badly wrong upstream. The wait is also clamped to the remaining budget. The budget is checked before sleeping, so a check passing at 4:59 would sleep a full Retry-After on top — at 12 hours that was a rounding error, at 5 minutes it doubled the bound. The new test waits 59.5s without the clamp and 1s with it. 135 unit tests, ruff clean, 61-test e2e suite passes. --- HISTORY.md | 3 ++ segment/analytics/client.py | 2 +- segment/analytics/consumer.py | 12 ++++++-- segment/analytics/request.py | 5 +++- segment/analytics/test/test_consumer.py | 37 +++++++++++++++++++++---- segment/analytics/test/test_request.py | 4 +-- 6 files changed, 52 insertions(+), 11 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index d858772e..836f3168 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,4 +1,7 @@ # Unreleased +- `max_rate_limit_duration` now defaults to 5 minutes rather than 12 hours, and `Retry-After` is capped at 60s rather than 300s. The 12 hour value was meant as a backstop that a retry count would stop us reaching, but rate-limited attempts are deliberately uncounted, so it was the operative limit — a server that kept sending `Retry-After` could hold one batch, and with a single consumer thread the whole pipeline, for half a day. Five minutes lines up with the counted-backoff path's ~4 minute worst case. `flush()` and `shutdown()` are bounded by the same figure. +- The rate-limit wait is clamped to the remaining budget. The budget is checked before waiting, so a check passing just inside it used to sleep a full `Retry-After` on top. + ### 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` diff --git a/segment/analytics/client.py b/segment/analytics/client.py index d5dc7e31..31b930f1 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -31,7 +31,7 @@ class DefaultConfig(object): timeout = 15 max_retries = 10 max_total_backoff_duration = 43200 - max_rate_limit_duration = 43200 + max_rate_limit_duration = 300 proxies = None thread = 1 upload_interval = 0.5 diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index deec68be..d1b259cd 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -24,7 +24,11 @@ class ShutdownInterrupted(Exception): # Default duration limits (12 hours in seconds) DEFAULT_MAX_TOTAL_BACKOFF_DURATION = 43200 -DEFAULT_MAX_RATE_LIMIT_DURATION = 43200 +# Five minutes, in line with the counted-backoff path's ~4 minute worst case. +# This was 12 hours, intended as a backstop that a retry count would stop us ever +# reaching — but nothing counts rate-limited attempts, so it was the operative +# limit rather than the backstop. +DEFAULT_MAX_RATE_LIMIT_DURATION = 300 class FatalError(Exception): @@ -166,7 +170,11 @@ def upload(self): # Still rate-limited; wait until the rate limit expires if self.rate_limited_until is not None: - wait_time = self.rate_limited_until - now + # Clamped to what is left of the budget. The check above runs before + # the wait, so without this a check passing at 4:59 would still sleep + # a full Retry-After on top and overshoot the budget. + remaining = self.max_rate_limit_duration - (now - self.rate_limit_start_time) + wait_time = min(self.rate_limited_until - now, remaining) if wait_time > 0: self.log.debug("Rate-limited. Waiting %.2fs before next upload attempt.", wait_time) if not self._wait(wait_time): diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 8f3ee0e0..257524b7 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -16,7 +16,10 @@ _session = sessions.Session() # Maximum Retry-After delay to respect (5 minutes) -MAX_RETRY_AFTER_SECONDS = 300 +# Capped well below max_rate_limit_duration so the budget buys several attempts +# rather than one long sleep. Segment serving a Retry-After longer than this would +# mean something has gone badly wrong upstream, so the cap costs nothing real. +MAX_RETRY_AFTER_SECONDS = 60 def parse_retry_after(response): diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 5bbc58b1..93e16234 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -323,8 +323,13 @@ def mock_post_fn(*args, **kwargs): # rate_limited_until should be ~10 seconds in the future 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""" + def test_retry_after_capped_at_60_seconds(self): + """Retry-After is clamped to MAX_RETRY_AFTER_SECONDS when setting rate-limit state. + + The cap sits well below max_rate_limit_duration on purpose: at the old 300s it + equalled the whole budget, so one sleep consumed it and the rate-limit path + gave a single attempt. + """ consumer = Consumer(None, "testsecret", retries=2) track = {"type": "track", "event": "python event", "userId": "userId"} @@ -340,10 +345,32 @@ def mock_post_fn(*args, **kwargs): with self.assertRaises(APIError): consumer.request([track]) - # rate_limited_until should be capped at ~300s from now (not 600s) + # rate_limited_until should be capped at ~60s 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) + self.assertLessEqual(consumer.rate_limited_until, now + 65) + self.assertGreater(consumer.rate_limited_until, now + 55) + + def test_rate_limit_wait_never_overshoots_the_budget(self): + """The wait is clamped to what is left of max_rate_limit_duration. + + The budget is checked before the wait, so without clamping a check passing + just inside the budget would still sleep a full Retry-After on top — turning + a 5 minute budget into 6. + """ + consumer = Consumer(Queue(), "testsecret", max_rate_limit_duration=300) + consumer.queue.put({"type": "track", "event": "e", "userId": "u"}) + + # An episode that began 299s ago: 1s of budget left, but Retry-After says 60. + consumer.rate_limit_start_time = time.monotonic() - 299 + consumer.rate_limited_until = time.monotonic() + 60 + + waits = [] + with mock.patch.object(Consumer, "_wait", side_effect=lambda s: waits.append(s) or True): + with mock.patch("segment.analytics.consumer.post", return_value=None): + consumer.upload() + + self.assertTrue(waits, "expected the consumer to wait") + self.assertLessEqual(waits[0], 1.5, f"waited {waits[0]}s with ~1s of budget left") def test_408_and_503_without_retry_after_use_backoff(self): """Test that 408 and 503 without Retry-After header use exponential backoff""" diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 2ac5f65e..75ba2a98 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -156,11 +156,11 @@ def test_parse_retry_after_integer(self): self.assertEqual(result, 30) def test_parse_retry_after_capped(self): - """Test that Retry-After is capped at 300 seconds""" + """Retry-After is capped at MAX_RETRY_AFTER_SECONDS (60s)""" response = mock.Mock() response.headers = {"Retry-After": "600"} result = parse_retry_after(response) - self.assertEqual(result, 300) + self.assertEqual(result, 60) def test_parse_retry_after_missing(self): """Test parsing when Retry-After header is missing""" From fe99426a82818ede141a2470a2669a3a5290bd15 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 17:53:09 -0400 Subject: [PATCH 02/10] Rewrite the release notes for a reader seeing them in isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three problems. The notes described changes between states that never shipped, so a customer read that a default moved from 12 hours to 5 minutes when only the 5 minutes was ever released. They referred to other SDKs, which means nothing to someone reading one library's notes. And they had accumulated over several passes into contradictions — Retry-After was documented as capped at both 300s and 60s, and the rate-limit budget as both 12 hours and 5 minutes. Rewritten to describe the behaviour this version has, in a consistent structure: upgrade notes that need action first, then retry handling, then everything else. Entries covering fixes to code that has not shipped are dropped, since there is nothing for a reader to compare against. --- HISTORY.md | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 836f3168..0748ca08 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,24 +1,26 @@ # Unreleased -- `max_rate_limit_duration` now defaults to 5 minutes rather than 12 hours, and `Retry-After` is capped at 60s rather than 300s. The 12 hour value was meant as a backstop that a retry count would stop us reaching, but rate-limited attempts are deliberately uncounted, so it was the operative limit — a server that kept sending `Retry-After` could hold one batch, and with a single consumer thread the whole pipeline, for half a day. Five minutes lines up with the counted-backoff path's ~4 minute worst case. `flush()` and `shutdown()` are bounded by the same figure. -- The rate-limit wait is clamped to the remaining budget. The budget is checked before waiting, so a check passing just inside it used to sleep a full `Retry-After` on top. -### Upgrade note: new request headers and proxy allowlists +### Upgrade note: new request headers + 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 +(sent on retries only). If traffic to Segment passes 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 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 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. +### Retry handling + +- Uploads are retried on 408, 410, 429, 460, and 5xx except 501 and 505. 511 is retried only when an `oauth_manager` is configured, and dropped otherwise. +- A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at 60 seconds. +- Responses carrying `Retry-After` are retried for up to `max_rate_limit_duration` and do not consume the retry count. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `max_retries` and by `max_total_backoff_duration` as an upper bound. +- New client options, both in seconds: `max_rate_limit_duration` (default 300) and `max_total_backoff_duration` (default 43200). +- `flush()` and `shutdown()` are bounded by the same limits, and a pending retry does not delay shutdown. + +### Other changes + +- The write key is sent as an `Authorization: Basic` header. It remains in the request body, so no server-side change is required. Deployments using OAuth continue to send `Authorization: Bearer`. +- `X-Retry-Count` is sent on retries, allowing the server to distinguish a retry from a first attempt. It is omitted on the first attempt. +- Only 2xx responses count as a successful upload. A 3xx is reported as a failed upload rather than treated as delivered, and is not retried. The Segment endpoint does not redirect, so this affects only custom `host` values. # 2.3.6 / 2026-4-7 - Update and widen PyJWT version to address security issue From a8b0ce4ae1721989178c9577af405e13b4ef6e6a Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 21:19:00 -0400 Subject: [PATCH 03/10] Cut the comments back to why, not history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the team convention to my own work from today. The comments explaining these changes had accumulated into potted histories: why a value had been twelve hours, what a test used to assert, which path used to be unreachable. Six months from now none of that resolves to anything — the diff and the commit messages hold it, and the comment should say why the code is the way it is. What stayed is what a maintainer would undo without it: that Kernel#sleep raises on a negative interval, that Thread#wakeup only interrupts a sleep already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an interrupt does not reach them, and that inverting one assertion would make the duration budget unreachable again. Comments only, no behaviour change. --- segment/analytics/consumer.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index d1b259cd..77f5bf67 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -24,10 +24,9 @@ class ShutdownInterrupted(Exception): # Default duration limits (12 hours in seconds) DEFAULT_MAX_TOTAL_BACKOFF_DURATION = 43200 -# Five minutes, in line with the counted-backoff path's ~4 minute worst case. -# This was 12 hours, intended as a backstop that a retry count would stop us ever -# reaching — but nothing counts rate-limited attempts, so it was the operative -# limit rather than the backstop. +# Rate-limited attempts are deliberately uncounted, so this duration is the only +# thing bounding them. Five minutes keeps that in line with the counted-backoff +# path's own worst case, so neither failure mode costs much more than the other. DEFAULT_MAX_RATE_LIMIT_DURATION = 300 From a75f43fc7fe5dec8588242c3773098bf762e0ebf Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 24 Sep 2026 09:54:07 -0400 Subject: [PATCH 04/10] Honour Retry-After up to 300s rather than capping it at 60 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capping at 60s meant waiting less than the server asked for, which does not make the next attempt more likely to succeed — it just sends more requests at something already rate-limiting us. Against a Retry-After of 180s inside a 5 minute budget it turns 3 requests into 6; against 300s it turns 2 into 6. The cap is a guard against an absurd header, not a second budget. How long we keep trying is max_rate_limit_duration's job, and the clamp to the remaining budget already stops a single wait running past it, so the cap now rarely binds at all. It also bought nothing for the client this was partly aimed at: with no background thread, a shorter cap turns one long wait into several short ones for the same total blocking time and more requests. Tests that pinned 60 are updated, and each SDK gains one asserting that a Retry-After inside the cap is used as given rather than shortened. --- HISTORY.md | 2 +- segment/analytics/request.py | 9 +++++---- segment/analytics/test/test_consumer.py | 15 +++++---------- segment/analytics/test/test_request.py | 4 ++-- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 0748ca08..362120f9 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -11,7 +11,7 @@ will be rejected. ### Retry handling - Uploads are retried on 408, 410, 429, 460, and 5xx except 501 and 505. 511 is retried only when an `oauth_manager` is configured, and dropped otherwise. -- A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at 60 seconds. +- A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at 300 seconds. - Responses carrying `Retry-After` are retried for up to `max_rate_limit_duration` and do not consume the retry count. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `max_retries` and by `max_total_backoff_duration` as an upper bound. - New client options, both in seconds: `max_rate_limit_duration` (default 300) and `max_total_backoff_duration` (default 43200). - `flush()` and `shutdown()` are bounded by the same limits, and a pending retry does not delay shutdown. diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 257524b7..28982aa7 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -16,10 +16,11 @@ _session = sessions.Session() # Maximum Retry-After delay to respect (5 minutes) -# Capped well below max_rate_limit_duration so the budget buys several attempts -# rather than one long sleep. Segment serving a Retry-After longer than this would -# mean something has gone badly wrong upstream, so the cap costs nothing real. -MAX_RETRY_AFTER_SECONDS = 60 +# A guard against an absurd header, not a second budget. Waiting less than the +# server asked for does not make the next attempt more likely to succeed, it just +# sends more requests at something already rate-limiting us; how long we keep +# trying is max_rate_limit_duration's job. +MAX_RETRY_AFTER_SECONDS = 300 def parse_retry_after(response): diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 93e16234..79a8a4d1 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -323,13 +323,8 @@ def mock_post_fn(*args, **kwargs): # rate_limited_until should be ~10 seconds in the future self.assertGreater(consumer.rate_limited_until, time.monotonic() + 5) - def test_retry_after_capped_at_60_seconds(self): - """Retry-After is clamped to MAX_RETRY_AFTER_SECONDS when setting rate-limit state. - - The cap sits well below max_rate_limit_duration on purpose: at the old 300s it - equalled the whole budget, so one sleep consumed it and the rate-limit path - gave a single attempt. - """ + def test_retry_after_capped_at_max_retry_after_seconds(self): + """Retry-After is clamped to MAX_RETRY_AFTER_SECONDS when setting rate-limit state.""" consumer = Consumer(None, "testsecret", retries=2) track = {"type": "track", "event": "python event", "userId": "userId"} @@ -345,10 +340,10 @@ def mock_post_fn(*args, **kwargs): with self.assertRaises(APIError): consumer.request([track]) - # rate_limited_until should be capped at ~60s from now, not 600s + # 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 + 65) - self.assertGreater(consumer.rate_limited_until, now + 55) + self.assertLessEqual(consumer.rate_limited_until, now + 310) + self.assertGreater(consumer.rate_limited_until, now + 290) def test_rate_limit_wait_never_overshoots_the_budget(self): """The wait is clamped to what is left of max_rate_limit_duration. diff --git a/segment/analytics/test/test_request.py b/segment/analytics/test/test_request.py index 75ba2a98..fb3dd245 100644 --- a/segment/analytics/test/test_request.py +++ b/segment/analytics/test/test_request.py @@ -156,11 +156,11 @@ def test_parse_retry_after_integer(self): self.assertEqual(result, 30) def test_parse_retry_after_capped(self): - """Retry-After is capped at MAX_RETRY_AFTER_SECONDS (60s)""" + """Retry-After is capped at MAX_RETRY_AFTER_SECONDS""" response = mock.Mock() response.headers = {"Retry-After": "600"} result = parse_retry_after(response) - self.assertEqual(result, 60) + self.assertEqual(result, 300) def test_parse_retry_after_missing(self): """Test parsing when Retry-After header is missing""" From 4f9928aa31a30fd977dac4a1e78a795833f30ffe Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 14:33:20 -0400 Subject: [PATCH 05/10] Raise the rate-limit budget to 30 minutes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The budget and the Retry-After cap were both 300s, and at parity the rate-limit path degenerates. A response with no usable Retry-After waits the cap by default, the elapsed check runs before the wait, so that one wait spends the whole budget and the batch is dropped having been tried once. A legitimate Retry-After of 300 does the same. The cap also stops binding: whatever is left of the budget is always the smaller term, so the cap can never be the value that clamps. Thirty minutes restores the relationship the two knobs are meant to have — the cap bounds one wait, the budget bounds the episode — and leaves room for several attempts. It costs nothing in normal operation, since the budget only binds when the server has been rate-limiting us for a long time, and in that case keeping the data is the point. --- HISTORY.md | 2 +- segment/analytics/consumer.py | 7 +++--- segment/analytics/test/test_consumer.py | 32 +++++++++++++++++++++++-- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 362120f9..5b603c78 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -13,7 +13,7 @@ will be rejected. - Uploads are retried on 408, 410, 429, 460, and 5xx except 501 and 505. 511 is retried only when an `oauth_manager` is configured, and dropped otherwise. - A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at 300 seconds. - Responses carrying `Retry-After` are retried for up to `max_rate_limit_duration` and do not consume the retry count. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `max_retries` and by `max_total_backoff_duration` as an upper bound. -- New client options, both in seconds: `max_rate_limit_duration` (default 300) and `max_total_backoff_duration` (default 43200). +- New client options, both in seconds: `max_rate_limit_duration` (default 1800) and `max_total_backoff_duration` (default 43200). - `flush()` and `shutdown()` are bounded by the same limits, and a pending retry does not delay shutdown. ### Other changes diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 77f5bf67..acf4424f 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -25,9 +25,10 @@ class ShutdownInterrupted(Exception): # Default duration limits (12 hours in seconds) DEFAULT_MAX_TOTAL_BACKOFF_DURATION = 43200 # Rate-limited attempts are deliberately uncounted, so this duration is the only -# thing bounding them. Five minutes keeps that in line with the counted-backoff -# path's own worst case, so neither failure mode costs much more than the other. -DEFAULT_MAX_RATE_LIMIT_DURATION = 300 +# thing bounding them. It is deliberately several times MAX_RETRY_AFTER_SECONDS: +# when the two are equal a single maximal Retry-After consumes the whole budget, +# leaving one attempt and no retry at all. +DEFAULT_MAX_RATE_LIMIT_DURATION = 1800 class FatalError(Exception): diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 79a8a4d1..d0fb6593 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -10,8 +10,13 @@ except ImportError: from Queue import Queue -from segment.analytics.consumer import MAX_MSG_SIZE, Consumer, FatalError -from segment.analytics.request import APIError +from segment.analytics.consumer import ( + DEFAULT_MAX_RATE_LIMIT_DURATION, + MAX_MSG_SIZE, + Consumer, + FatalError, +) +from segment.analytics.request import MAX_RETRY_AFTER_SECONDS, APIError class TestConsumer(unittest.TestCase): @@ -1153,3 +1158,26 @@ def mock_post_fn(*args, **kwargs): self.assertEqual(error.status, 500) self.assertEqual(len(batch), 1) self.assertEqual(batch[0]["event"], "test event") + + def test_default_rate_limit_budget_exceeds_the_retry_after_cap(self): + """The budget must leave room for more than one maximal Retry-After. + + The elapsed check runs before the wait, so at parity a single capped + Retry-After spends the whole budget and the batch is dropped having been + attempted once, with no retry at all. + """ + self.assertGreater( + DEFAULT_MAX_RATE_LIMIT_DURATION, + MAX_RETRY_AFTER_SECONDS, + "a capped Retry-After would consume the entire rate-limit budget", + ) + self.assertGreaterEqual( + DEFAULT_MAX_RATE_LIMIT_DURATION // MAX_RETRY_AFTER_SECONDS, + 2, + "budget leaves room for fewer than two capped waits", + ) + + def test_consumer_defaults_to_the_documented_rate_limit_budget(self): + """Pins the default the changelog advertises; nothing else asserts it.""" + consumer = Consumer(Queue(), "testsecret") + self.assertEqual(consumer.max_rate_limit_duration, DEFAULT_MAX_RATE_LIMIT_DURATION) From da0b7010f736b6f461db107af6f159cfcf281728 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 15:21:36 -0400 Subject: [PATCH 06/10] End the rate-limit episode when a request fails for another reason rate_limit_start_time marks an episode and is deliberately not cleared when a wait is served, so the budget measures the whole episode. But it was only cleared on success and on budget exhaustion. A request that completed and carried no rate-limit signal -- a 400, a 401 after a key rotation, a network error that exhausted counted retries -- left it set. The consumer outlives the batch, and upload() returns at the empty-batch guard before the budget block, so nothing else clears it. The next batch to arrive after the budget elapses is then dropped with "Rate limit duration exceeded", having never been sent and never been rate-limited. This is the only SDK where it can happen: go, ruby and php scope the episode to one batch and java clears it after the retry loop. The 12h default made it effectively unreachable; at 30 minutes a quiet app reaches it. Also widened the clamp test's margin. It sat 1s from the end of the budget, but next() blocks out the rest of upload_interval before the budget check, so any scheduling stall sent it down the drop path and failed on an empty `waits` -- a confusing failure for an unrelated reason. It now sits 30s out and also asserts the wait is non-zero, which the upper bound alone did not. 139 passed, ruff and format clean. --- segment/analytics/consumer.py | 9 ++++ segment/analytics/test/test_consumer.py | 61 +++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index acf4424f..83debe93 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -209,11 +209,20 @@ def upload(self): self._requeue(batch) success = False else: + # The request completed and carried no rate-limit signal, so the + # episode is over. Leaving the marker set strands it: this consumer + # outlives the batch, upload() returns at the empty-batch guard + # before the budget block, and nothing else clears it — so the next + # batch to arrive after the budget elapses is dropped for a rate + # limit that ended here, without ever being sent. + self.clear_rate_limit_state() self.log.error("error uploading: %s", e) success = False if self.on_error: self.on_error(e, batch) except Exception as e: + # Same reasoning as above. + self.clear_rate_limit_state() self.log.error("error uploading: %s", e) success = False if self.on_error: diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index d0fb6593..d220d7d5 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -360,8 +360,11 @@ def test_rate_limit_wait_never_overshoots_the_budget(self): consumer = Consumer(Queue(), "testsecret", max_rate_limit_duration=300) consumer.queue.put({"type": "track", "event": "e", "userId": "u"}) - # An episode that began 299s ago: 1s of budget left, but Retry-After says 60. - consumer.rate_limit_start_time = time.monotonic() - 299 + # An episode 30s from its end, against a Retry-After of 60. Deliberately not + # placed 1s from the end: next() blocks out the rest of upload_interval + # before the budget check runs, so a margin that tight turns any scheduling + # stall into a spurious budget-exceeded drop and an empty `waits`. + consumer.rate_limit_start_time = time.monotonic() - 270 consumer.rate_limited_until = time.monotonic() + 60 waits = [] @@ -370,7 +373,8 @@ def test_rate_limit_wait_never_overshoots_the_budget(self): consumer.upload() self.assertTrue(waits, "expected the consumer to wait") - self.assertLessEqual(waits[0], 1.5, f"waited {waits[0]}s with ~1s of budget left") + self.assertGreater(waits[0], 0, "a wait of 0 would pass any upper bound vacuously") + self.assertLessEqual(waits[0], 31, f"waited {waits[0]}s with ~30s of budget left") def test_408_and_503_without_retry_after_use_backoff(self): """Test that 408 and 503 without Retry-After header use exponential backoff""" @@ -1181,3 +1185,54 @@ def test_consumer_defaults_to_the_documented_rate_limit_budget(self): """Pins the default the changelog advertises; nothing else asserts it.""" consumer = Consumer(Queue(), "testsecret") self.assertEqual(consumer.max_rate_limit_duration, DEFAULT_MAX_RATE_LIMIT_DURATION) + + def test_non_rate_limited_failure_ends_the_episode(self): + """A request that completed without a rate-limit signal ends the episode. + + The marker outlives the batch that opened it, so leaving it set strands it: + upload() returns at the empty-batch guard before the budget block, and + nothing else clears it. + """ + q = Queue() + q.put({"event": "one"}) + consumer = Consumer(q, "testsecret", on_error=lambda e, b: None) + consumer.rate_limit_start_time = time.monotonic() + + with mock.patch( + "segment.analytics.consumer.post", + side_effect=APIError(400, "invalid", "Bad Request"), + ): + consumer.upload() + + self.assertIsNone( + consumer.rate_limit_start_time, + "a completed request carrying no rate-limit signal must end the episode", + ) + + def test_batch_after_an_ended_episode_is_still_sent(self): + """The symptom: a stranded marker drops a batch that was never rate-limited.""" + q = Queue() + consumer = Consumer( + q, "testsecret", max_rate_limit_duration=1, on_error=lambda e, b: None + ) + consumer.rate_limit_start_time = time.monotonic() + + q.put({"event": "one"}) + with mock.patch( + "segment.analytics.consumer.post", + side_effect=APIError(400, "invalid", "Bad Request"), + ): + consumer.upload() + + time.sleep(1.1) # longer than this consumer's 1 second budget + + sent = [] + q.put({"event": "two"}) + with mock.patch( + "segment.analytics.consumer.post", side_effect=lambda *a, **k: sent.append(1) + ): + consumer.upload() + + self.assertEqual( + len(sent), 1, "batch was dropped for a rate-limit episode that had already ended" + ) From f267740b11de8133a80a9e8e48343777039f9e94 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 16:57:58 -0400 Subject: [PATCH 07/10] Fix the client default, and end an episode rather than retry inside its window Two things from review. Client.DefaultConfig carried its own literal 300 and Client passes it into every Consumer it builds, so raising only the Consumer default left every real caller on the old value -- which equals the Retry-After cap, the exact configuration the change was meant to eliminate. HISTORY advertised 1800 and the suite was green. Both DefaultConfig entries now derive from the consumer constants, so they cannot drift apart again. The test that was supposed to cover this constructed a Consumer directly and so pinned the one default a caller never gets. It now goes through Client, and fails with "300 != 1800" against the old code. The remaining-budget clamp is replaced by a drop. Shortening a Retry-After to fit the budget sends the next request inside the window the server asked us to wait out -- a request it has already said it will not serve -- and since the budget is spent by then it would be the final attempt regardless. So the clamp bought one guaranteed-refused request per episode. Giving up at that point loses the same batch and sends one request fewer at a server that is already rate-limiting us. The clamp test became a pair: one that a wait which cannot fit drops without waiting or posting, and one that a wait which does fit is still honoured in full, since "never shorten" must not become "never wait". 140 passed, ruff and format clean. --- HISTORY.md | 2 +- segment/analytics/client.py | 11 +++-- segment/analytics/consumer.py | 21 +++++++-- segment/analytics/test/test_consumer.py | 61 +++++++++++++++++++------ 4 files changed, 72 insertions(+), 23 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 5b603c78..dc032fa8 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -12,7 +12,7 @@ will be rejected. - Uploads are retried on 408, 410, 429, 460, and 5xx except 501 and 505. 511 is retried only when an `oauth_manager` is configured, and dropped otherwise. - A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at 300 seconds. -- Responses carrying `Retry-After` are retried for up to `max_rate_limit_duration` and do not consume the retry count. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `max_retries` and by `max_total_backoff_duration` as an upper bound. +- Responses carrying `Retry-After` are retried for up to `max_rate_limit_duration` and do not consume the retry count. A `Retry-After` that will not fit in what is left of the budget ends the episode rather than being shortened: retrying inside the window the server asked for sends a request it has already declined to serve, and the budget would be spent by then anyway. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `max_retries` and by `max_total_backoff_duration` as an upper bound. - New client options, both in seconds: `max_rate_limit_duration` (default 1800) and `max_total_backoff_duration` (default 43200). - `flush()` and `shutdown()` are bounded by the same limits, and a pending retry does not delay shutdown. diff --git a/segment/analytics/client.py b/segment/analytics/client.py index 31b930f1..50630814 100644 --- a/segment/analytics/client.py +++ b/segment/analytics/client.py @@ -8,7 +8,12 @@ from dateutil.tz import tzutc -from segment.analytics.consumer import MAX_MSG_SIZE, Consumer +from segment.analytics.consumer import ( + DEFAULT_MAX_RATE_LIMIT_DURATION, + DEFAULT_MAX_TOTAL_BACKOFF_DURATION, + MAX_MSG_SIZE, + Consumer, +) from segment.analytics.oauth_manager import OauthManager from segment.analytics.request import DatetimeSerializer, post from segment.analytics.utils import clean, guess_timezone @@ -30,8 +35,8 @@ class DefaultConfig(object): gzip = False timeout = 15 max_retries = 10 - max_total_backoff_duration = 43200 - max_rate_limit_duration = 300 + max_total_backoff_duration = DEFAULT_MAX_TOTAL_BACKOFF_DURATION + max_rate_limit_duration = DEFAULT_MAX_RATE_LIMIT_DURATION proxies = None thread = 1 upload_interval = 0.5 diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 83debe93..2808da29 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -170,11 +170,24 @@ def upload(self): # Still rate-limited; wait until the rate limit expires if self.rate_limited_until is not None: - # Clamped to what is left of the budget. The check above runs before - # the wait, so without this a check passing at 4:59 would still sleep - # a full Retry-After on top and overshoot the budget. remaining = self.max_rate_limit_duration - (now - self.rate_limit_start_time) - wait_time = min(self.rate_limited_until - now, remaining) + wait_time = self.rate_limited_until - now + if wait_time > remaining: + # Shortening the wait to fit the budget would send the next request + # inside the window the server asked us to wait out — a request it + # has already said it will not serve — and the budget would then be + # spent, so it would be the last one anyway. Give up here instead of + # spending a request to be told the same thing. + self.log.error( + "Rate limit budget (%ds) cannot accommodate the requested wait; dropping batch.", + self.max_rate_limit_duration, + ) + self.clear_rate_limit_state() + if self.on_error: + self.on_error(Exception("Rate limit duration exceeded, batch dropped"), batch) + for _ in batch: + self.queue.task_done() + return False if wait_time > 0: self.log.debug("Rate-limited. Waiting %.2fs before next upload attempt.", wait_time) if not self._wait(wait_time): diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index d220d7d5..12bdeb0d 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -10,6 +10,7 @@ except ImportError: from Queue import Queue +from segment.analytics.client import Client from segment.analytics.consumer import ( DEFAULT_MAX_RATE_LIMIT_DURATION, MAX_MSG_SIZE, @@ -350,31 +351,50 @@ def mock_post_fn(*args, **kwargs): self.assertLessEqual(consumer.rate_limited_until, now + 310) self.assertGreater(consumer.rate_limited_until, now + 290) - def test_rate_limit_wait_never_overshoots_the_budget(self): - """The wait is clamped to what is left of max_rate_limit_duration. + def test_a_wait_that_will_not_fit_the_budget_drops_instead_of_shortening(self): + """Never retry inside the window the server asked us to wait out. - The budget is checked before the wait, so without clamping a check passing - just inside the budget would still sleep a full Retry-After on top — turning - a 5 minute budget into 6. + Shortening the wait to fit the budget sends the next request before the + server said it would serve one, and the budget is spent by then, so it + would be the last attempt regardless. Dropping here costs the same batch + and one fewer request against a server already rate-limiting us. """ consumer = Consumer(Queue(), "testsecret", max_rate_limit_duration=300) consumer.queue.put({"type": "track", "event": "e", "userId": "u"}) - # An episode 30s from its end, against a Retry-After of 60. Deliberately not - # placed 1s from the end: next() blocks out the rest of upload_interval - # before the budget check runs, so a margin that tight turns any scheduling - # stall into a spurious budget-exceeded drop and an empty `waits`. + errors = [] + consumer.on_error = lambda e, b: errors.append(e) + + # 30s of budget left against a Retry-After of 60: it cannot fit. consumer.rate_limit_start_time = time.monotonic() - 270 consumer.rate_limited_until = time.monotonic() + 60 + waits = [] + posts = [] + with mock.patch.object(Consumer, "_wait", side_effect=lambda s: waits.append(s) or True): + with mock.patch("segment.analytics.consumer.post", side_effect=lambda *a, **k: posts.append(1)): + consumer.upload() + + self.assertEqual(waits, [], "should not have waited a shortened interval") + self.assertEqual(posts, [], "should not have sent a request inside the Retry-After window") + self.assertEqual(len(errors), 1, "the dropped batch must be reported") + + def test_a_wait_that_fits_the_budget_is_honoured_in_full(self): + """The counterpart: a wait that fits is taken as the server asked for it.""" + consumer = Consumer(Queue(), "testsecret", max_rate_limit_duration=300) + consumer.queue.put({"type": "track", "event": "e", "userId": "u"}) + + # 120s of budget left against a Retry-After of 60: it fits. + consumer.rate_limit_start_time = time.monotonic() - 180 + consumer.rate_limited_until = time.monotonic() + 60 + waits = [] with mock.patch.object(Consumer, "_wait", side_effect=lambda s: waits.append(s) or True): with mock.patch("segment.analytics.consumer.post", return_value=None): consumer.upload() self.assertTrue(waits, "expected the consumer to wait") - self.assertGreater(waits[0], 0, "a wait of 0 would pass any upper bound vacuously") - self.assertLessEqual(waits[0], 31, f"waited {waits[0]}s with ~30s of budget left") + self.assertGreater(waits[0], 55, f"waited {waits[0]}s; the full Retry-After should be honoured") def test_408_and_503_without_retry_after_use_backoff(self): """Test that 408 and 503 without Retry-After header use exponential backoff""" @@ -1181,10 +1201,21 @@ def test_default_rate_limit_budget_exceeds_the_retry_after_cap(self): "budget leaves room for fewer than two capped waits", ) - def test_consumer_defaults_to_the_documented_rate_limit_budget(self): - """Pins the default the changelog advertises; nothing else asserts it.""" - consumer = Consumer(Queue(), "testsecret") - self.assertEqual(consumer.max_rate_limit_duration, DEFAULT_MAX_RATE_LIMIT_DURATION) + def test_client_defaults_to_the_documented_rate_limit_budget(self): + """Pins what a real caller gets, which is not the same as Consumer's default. + + Client.DefaultConfig carried its own literal and Client passes it into every + Consumer it builds, so raising only the Consumer default left every real user + on the old value while this suite stayed green. + """ + client = Client("testsecret", send=False) + try: + self.assertEqual( + client.consumers[0].max_rate_limit_duration, + DEFAULT_MAX_RATE_LIMIT_DURATION, + ) + finally: + client.shutdown() def test_non_rate_limited_failure_ends_the_episode(self): """A request that completed without a rate-limit signal ends the episode. From 7873174255bcb45b2acc0ce43b67754b55f29a2d Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 18:46:10 -0400 Subject: [PATCH 08/10] Apply ruff format to the tests added in this branch The CI Lint job runs ruff format --check, which this file failed: the repo's line length is 140 and the new tests were hand-wrapped tighter. No behaviour change. Caught by CI rather than locally because the format check shared a shell line with the commit that followed it, so its output was never actually read. --- segment/analytics/test/test_consumer.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index 12bdeb0d..e4743acd 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -1243,9 +1243,7 @@ def test_non_rate_limited_failure_ends_the_episode(self): def test_batch_after_an_ended_episode_is_still_sent(self): """The symptom: a stranded marker drops a batch that was never rate-limited.""" q = Queue() - consumer = Consumer( - q, "testsecret", max_rate_limit_duration=1, on_error=lambda e, b: None - ) + consumer = Consumer(q, "testsecret", max_rate_limit_duration=1, on_error=lambda e, b: None) consumer.rate_limit_start_time = time.monotonic() q.put({"event": "one"}) @@ -1259,11 +1257,7 @@ def test_batch_after_an_ended_episode_is_still_sent(self): sent = [] q.put({"event": "two"}) - with mock.patch( - "segment.analytics.consumer.post", side_effect=lambda *a, **k: sent.append(1) - ): + with mock.patch("segment.analytics.consumer.post", side_effect=lambda *a, **k: sent.append(1)): consumer.upload() - self.assertEqual( - len(sent), 1, "batch was dropped for a rate-limit episode that had already ended" - ) + self.assertEqual(len(sent), 1, "batch was dropped for a rate-limit episode that had already ended") From e3785d2154c59e1966f8af9ef5d42947f3487d96 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 18:52:26 -0400 Subject: [PATCH 09/10] Comment cleanup pass Against the team convention: say why not what, no archaeology, keep a warning only where it stops someone undoing the thing it guards. - DEFAULT_MAX_RATE_LIMIT_DURATION: states the invariant as an instruction to whoever changes it next, and adds the second consequence of parity -- the cap stops binding, because the remaining budget is always the smaller term. - The drop branch: cut the rhetorical tail; the reason stands without it. - The episode-clearing comment: reordered so the hazard leads and the mechanism supports it, rather than the other way round. - "Same reasoning as above" now names the branch it refers to. - request.py: dropped a line that restated the constant in words, directly above the line that gives its reason. - test_client_defaults_to_the_documented_rate_limit_budget: rewritten in the present tense. It described how the bug had happened; it now states the constraint that makes asserting Consumer's default insufficient, which is what stops the test being "simplified" back. 140 passed, ruff and format clean. --- segment/analytics/consumer.py | 29 ++++++++++++------------- segment/analytics/request.py | 1 - segment/analytics/test/test_consumer.py | 6 ++--- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 2808da29..1e97d98d 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -25,9 +25,9 @@ class ShutdownInterrupted(Exception): # Default duration limits (12 hours in seconds) DEFAULT_MAX_TOTAL_BACKOFF_DURATION = 43200 # Rate-limited attempts are deliberately uncounted, so this duration is the only -# thing bounding them. It is deliberately several times MAX_RETRY_AFTER_SECONDS: -# when the two are equal a single maximal Retry-After consumes the whole budget, -# leaving one attempt and no retry at all. +# thing bounding them. Keep it several times MAX_RETRY_AFTER_SECONDS: at parity a +# single maximal Retry-After consumes the whole budget, leaving one attempt and no +# retry, and the cap stops binding because the remaining budget is always smaller. DEFAULT_MAX_RATE_LIMIT_DURATION = 1800 @@ -173,11 +173,10 @@ def upload(self): remaining = self.max_rate_limit_duration - (now - self.rate_limit_start_time) wait_time = self.rate_limited_until - now if wait_time > remaining: - # Shortening the wait to fit the budget would send the next request - # inside the window the server asked us to wait out — a request it - # has already said it will not serve — and the budget would then be - # spent, so it would be the last one anyway. Give up here instead of - # spending a request to be told the same thing. + # Shortening the wait to fit sends the next request inside the + # window the server asked us to wait out, which it has already said + # it will not serve, and the budget is spent by then so it would be + # the last attempt either way. self.log.error( "Rate limit budget (%ds) cannot accommodate the requested wait; dropping batch.", self.max_rate_limit_duration, @@ -222,19 +221,19 @@ def upload(self): self._requeue(batch) success = False else: - # The request completed and carried no rate-limit signal, so the - # episode is over. Leaving the marker set strands it: this consumer - # outlives the batch, upload() returns at the empty-batch guard - # before the budget block, and nothing else clears it — so the next - # batch to arrive after the budget elapses is dropped for a rate - # limit that ended here, without ever being sent. + # The request completed carrying no rate-limit signal, so the + # episode is over. The marker outlives the batch and nothing else + # clears it — upload() returns at the empty-batch guard above, + # before the budget block — so leaving it set means the next batch + # to arrive after the budget elapses is dropped for a rate limit + # that ended here, without ever being sent. self.clear_rate_limit_state() self.log.error("error uploading: %s", e) success = False if self.on_error: self.on_error(e, batch) except Exception as e: - # Same reasoning as above. + # Same reasoning as the non-rate-limited APIError branch above. self.clear_rate_limit_state() self.log.error("error uploading: %s", e) success = False diff --git a/segment/analytics/request.py b/segment/analytics/request.py index 28982aa7..09243ca9 100644 --- a/segment/analytics/request.py +++ b/segment/analytics/request.py @@ -15,7 +15,6 @@ _session = sessions.Session() -# Maximum Retry-After delay to respect (5 minutes) # A guard against an absurd header, not a second budget. Waiting less than the # server asked for does not make the next attempt more likely to succeed, it just # sends more requests at something already rate-limiting us; how long we keep diff --git a/segment/analytics/test/test_consumer.py b/segment/analytics/test/test_consumer.py index e4743acd..e868b0b8 100644 --- a/segment/analytics/test/test_consumer.py +++ b/segment/analytics/test/test_consumer.py @@ -1204,9 +1204,9 @@ def test_default_rate_limit_budget_exceeds_the_retry_after_cap(self): def test_client_defaults_to_the_documented_rate_limit_budget(self): """Pins what a real caller gets, which is not the same as Consumer's default. - Client.DefaultConfig carried its own literal and Client passes it into every - Consumer it builds, so raising only the Consumer default left every real user - on the old value while this suite stayed green. + Client passes its own DefaultConfig value into every Consumer it builds, so + asserting Consumer's default here would stay green while a drifted Client + default shipped. """ client = Client("testsecret", send=False) try: From 44646e98e5a1960f2e2237df2d78e9bf7395722c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 19:23:28 -0400 Subject: [PATCH 10/10] Cut the control-flow trace out of the episode-clearing comment The why and the hazard earn their place; the clauses walking through upload()'s control flow to connect them do not, since the reader can follow the code. Six lines to four, same two facts. --- segment/analytics/consumer.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/segment/analytics/consumer.py b/segment/analytics/consumer.py index 1e97d98d..b12f0f48 100644 --- a/segment/analytics/consumer.py +++ b/segment/analytics/consumer.py @@ -221,12 +221,10 @@ def upload(self): self._requeue(batch) success = False else: - # The request completed carrying no rate-limit signal, so the - # episode is over. The marker outlives the batch and nothing else - # clears it — upload() returns at the empty-batch guard above, - # before the budget block — so leaving it set means the next batch - # to arrive after the budget elapses is dropped for a rate limit - # that ended here, without ever being sent. + # The request completed carrying no rate-limit signal, so the episode + # is over. Nothing else clears the marker, so leaving it set drops the + # next batch to arrive after the budget elapses — for a rate limit that + # ended here, without ever sending it. self.clear_rate_limit_state() self.log.error("error uploading: %s", e) success = False