From eb2c8096c849deba9172f9776b8a5290af173abb Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 16:44:35 -0400 Subject: [PATCH 01/10] Bound the rate-limit budget at 5 minutes instead of 12 hours The 12 hour default was a backstop on the assumption a retry count would stop us reaching it. Rate-limited attempts are deliberately uncounted, so it was the operative limit instead. That matters more here than in the other clients. LibCurl is the default consumer and there is no background thread: flushBatch retries inline, so the budget is time the caller's web request spends blocked and an FPM worker spends occupied. Under load that is pool exhaustion rather than late telemetry. Five minutes matches the counted path's ~4 minute worst case. rate_limit_retry_after_cap drops to 60s: at 300s it equalled the whole budget, so one sleep consumed it and the path gave a single attempt. The sleep is clamped to the remaining budget, since the elapsed check runs before the wait. The new test sleeps 60s without the clamp and 1s with it. Exhausting the budget now logs unconditionally. handleError only writes when debug is on, and debug defaults to false, so the one failure mode that blocks a request for minutes produced no output at all. 83 unit tests (the same 2 pre-existing failures), phpcs clean, 61-test e2e suite passes. --- HISTORY.md | 4 ++++ lib/Consumer/LibCurl.php | 22 ++++++++++++++++++++-- lib/Consumer/QueueConsumer.php | 17 +++++++++++++++-- test/ConsumerLibCurlTest.php | 29 +++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 035731c..36d6ada 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,6 +1,10 @@ Unreleased ================== + * `max_rate_limit_duration` now defaults to 5 minutes rather than 12 hours, and `rate_limit_retry_after_cap` to 60s rather than 300s. The 12 hour value was a backstop meant to be unreachable, but rate-limited attempts are deliberately uncounted, so it was the only limit on that path. It matters more here than in the other clients: the LibCurl consumer is the default and retries inline on the caller's thread, so that budget is time a web request spends blocked and an FPM worker spends occupied. + * The rate-limit sleep is clamped to the remaining budget. The elapsed check runs before the wait, so a check passing just inside the budget previously slept a full `Retry-After` on top. + * Exhausting the rate-limit budget now logs unconditionally rather than only when `debug` is on. A request that stopped for the whole budget should not have to be diagnosed from an absence of output. + ### Upgrade note: new request header and proxy allowlists This release sends an `X-Retry-Count` request header on retries. If your diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index eca02bd..7d5ab82 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -95,10 +95,28 @@ public function flushBatch(array $messages): bool // expire or extend this budget. $rateLimitStartTime = hrtime(true); } - if ((hrtime(true) - $rateLimitStartTime) / 1e6 >= $this->max_rate_limit_duration_ms) { + $elapsedMs = (hrtime(true) - $rateLimitStartTime) / 1e6; + if ($elapsedMs >= $this->max_rate_limit_duration_ms) { + // Logged unconditionally: this consumer blocks the caller, and a + // request that stopped for the whole budget should not have to be + // diagnosed from an absence of output. handleError only writes when + // debug is on, which it is not by default. + error_log(sprintf( + '[Analytics][%s] Rate-limit budget of %dms exhausted; dropping batch', + $this->type, + $this->max_rate_limit_duration_ms + )); return false; } - $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); + // Clamped to the remaining budget as well as the cap: the elapsed check + // above runs before the wait, so without this a check passing just inside + // the budget would sleep a full Retry-After on top and overshoot it. + $remainingMs = (int)($this->max_rate_limit_duration_ms - $elapsedMs); + $sleepMs = min( + $retryAfterS * 1000, + $this->rate_limit_retry_after_cap_s * 1000, + $remainingMs + ); $this->sleepBeforeRetry($sleepMs, true); continue; // Do NOT decrement retriesRemaining } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 281d49c..a38bbb7 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -20,8 +20,21 @@ abstract class QueueConsumer extends Consumer protected int $max_item_size_bytes = 32000; // 32kb protected int $maximum_backoff_duration = 10000; // Set maximum waiting limit to 10s protected int $max_total_backoff_duration_ms = 43200000; // 12 hours - protected int $max_rate_limit_duration_ms = 43200000; // 12 hours - protected int $rate_limit_retry_after_cap_s = 300; // 5 minutes + + /** + * Five minutes, in line with the counted-backoff path's ~4 minute worst case. + * This was 12 hours, meant as a backstop a retry count would stop us reaching — + * but rate-limited attempts are deliberately uncounted, so it was the only limit + * on that path. This consumer retries inline on the caller's thread, so that + * budget is time a web request spends blocked. + */ + protected int $max_rate_limit_duration_ms = 300000; // 5 minutes + + /** + * Kept well below max_rate_limit_duration so the budget buys several attempts + * rather than one long sleep; at the old 300s a single sleep consumed it. + */ + protected int $rate_limit_retry_after_cap_s = 60; protected int $retry_count = 10; // max retries protected string $host = ''; protected bool $compress_request = false; diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index cd2e8d1..3be8cc2 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -275,6 +275,35 @@ public function testRetryCountOfZeroGrantsNoRetries(): void self::assertCount(0, $consumer->sleepCalls); } + public function testRateLimitSleepNeverOvershootsTheBudget(): void + { + // The elapsed check runs before the wait, so without clamping a check + // passing just inside the budget sleeps a full Retry-After on top. At a + // 5 minute budget that doubles the bound rather than rounding it, and + // this consumer blocks the caller for the whole of it. + $consumer = new MockLibCurl('test-secret', [ + 'max_rate_limit_duration' => 1, // 1 second of budget + 'retry_count' => 5, + ]); + + $consumer->responses = [ + [429, ['retry-after' => '60'], 'Too Many Requests', ''], + [429, ['retry-after' => '60'], 'Too Many Requests', ''], + [200, [], '{"success":true}', ''], + ]; + + $consumer->flushBatch(makeTestMessages()); + + self::assertNotEmpty($consumer->sleepCalls); + foreach ($consumer->sleepCalls as $micros) { + self::assertLessThanOrEqual( + 1_000_000, + $micros, + 'slept ' . $micros . 'us with at most 1s of budget left' + ); + } + } + public function testNegativeBudgetOptionsKeepTheDefault(): void { // A negative value used to be cast straight in, which silently disabled From ee8c33e1fe266af9e026d169b281e9abcfdfec9d 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 | 52 ++++++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 36d6ada..94db9ec 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,31 +1,35 @@ Unreleased ================== - * `max_rate_limit_duration` now defaults to 5 minutes rather than 12 hours, and `rate_limit_retry_after_cap` to 60s rather than 300s. The 12 hour value was a backstop meant to be unreachable, but rate-limited attempts are deliberately uncounted, so it was the only limit on that path. It matters more here than in the other clients: the LibCurl consumer is the default and retries inline on the caller's thread, so that budget is time a web request spends blocked and an FPM worker spends occupied. - * The rate-limit sleep is clamped to the remaining budget. The elapsed check runs before the wait, so a check passing just inside the budget previously slept a full `Retry-After` on top. - * Exhausting the rate-limit budget now logs unconditionally rather than only when `debug` is on. A request that stopped for the whole budget should not have to be diagnosed from an absence of output. - -### Upgrade note: new request header and proxy allowlists - -This release sends an `X-Retry-Count` request header on retries. If your -traffic to Segment goes through a proxy, gateway or WAF that allowlists -request headers, add it before upgrading or retried uploads will be -rejected. The `Authorization` header is unchanged: this client has always -sent the write key as HTTP Basic credentials. - - * Send `X-Retry-Count` on retries from both the LibCurl and Socket consumers, 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_limit_retry_after_cap`). Malformed values are rejected rather than parsed into an arbitrary date. - * 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 options `max_total_backoff_duration` and `max_rate_limit_duration`, both in seconds and defaulting to 12 hours, bound the two waits. - * The Socket consumer now honours `retry_count` and `max_total_backoff_duration` too, and backs off from 500ms like the LibCurl consumer rather than 100ms. It previously ignored both and gave up after a fixed seven retries over roughly 13 seconds. `maximum_backoff_duration` now caps each individual wait rather than ending the loop, so its 10s default still bounds how long any one retry sleeps. `max_rate_limit_duration` does not apply there, since Socket still does not read `Retry-After` — use the LibCurl consumer if you need that. - * `retry_count` grants exactly that many retries. It previously granted one fewer, and a `retry_count` of 1 granted none. - * The new budget options reject negative values and keep the default, logging as `flush_at` and `flush_interval` already do. A negative previously disabled retrying outright. Zero is still accepted and meaningful: `retry_count` of 0 means do not retry. - * Transport failures report the real libcurl error number to `error_handler` again, so a DNS failure, a timeout and a TLS error can be told apart. - * 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 curl already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom `host` values. - * Retry timing uses `hrtime()`, so a system clock change cannot stretch or collapse a backoff. - * Fix an oversized batch wedging the queue: the batch is now removed before the size check, so one too-large batch no longer makes every later `track()` return false. +### Upgrade note: new request header +This release sends an `X-Retry-Count` request header on retries. If traffic to +Segment passes through a proxy, gateway or WAF that allowlists request headers, +add it before upgrading or retried uploads will be rejected. The `Authorization` +header is unchanged. + +### Upgrade note: Socket consumer retry behaviour + +The Socket consumer previously gave up after a fixed seven retries over roughly +13 seconds. It now honours `retry_count` and `max_total_backoff_duration`, and +backs off from 500ms rather than 100ms, so a failing upload is retried for +longer than before. `maximum_backoff_duration` now caps each individual wait +rather than ending the retry loop; its 10 second default still bounds how long +any one retry sleeps. Lower `retry_count` to restore a shorter schedule. + +### Retry handling + + * Uploads are retried on 408, 410, 429, 460, and 5xx except 501, 505 and 511. + * 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, malformed values are ignored, and the value is capped at `rate_limit_retry_after_cap`. + * 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 limited by `retry_count` and by `max_total_backoff_duration` as an upper bound. + * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 60). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. + * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. + * Exhausting the rate-limit budget is logged regardless of the `debug` setting. + +### Other changes + + * `X-Retry-Count` is sent on retries by the LibCurl and Socket consumers, 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: a redirect curl has already declined to follow will not succeed on one. The Segment endpoint does not redirect, so this affects only custom `host` values. 3.8.2 / 2026-03-11 ================== From 475670b9e2a2c5ae0d5238c83098f17538940610 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 19:29:05 -0400 Subject: [PATCH 03/10] Fix a cap test that the remaining-budget clamp broke MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testRetryAfterCapIsRespected passed rate_limit_retry_after_cap of 300 while leaving max_rate_limit_duration at its default — which this branch also changed to 300 seconds. With the cap and the budget equal, the new clamp correctly picks the remaining budget, which is short of the cap by however much time has already elapsed. The assertion failed by 1ms. The test is about the cap, so the budget is now raised well above it. A second test covers the shipped default, where a large Retry-After clamps to 60 seconds well inside the budget. I introduced this and missed it: after adding the clamp test I ran only that one by filter, and reported the suite as green on the strength of an earlier run. The full suite is 85 tests with the same 2 pre-existing failures — a File consumer test needing the send script and a Socket timeout test. phpcs clean, 61-test e2e suite passes. --- test/ConsumerLibCurlTest.php | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 3be8cc2..8cd960c 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -481,9 +481,14 @@ public function testOversizedBatchDoesNotWedgeTheQueue(): void public function testRetryAfterCapIsRespected(): void { + // max_rate_limit_duration is raised well above the cap so the cap is what + // binds. Left at its default it equals the cap, and the remaining-budget + // clamp then wins by the fraction of a millisecond that has already + // elapsed — which is correct behaviour but tests the wrong thing. $consumer = new MockLibCurl('test-secret', [ 'retry_count' => 3, 'rate_limit_retry_after_cap' => 300, + 'max_rate_limit_duration' => 3600, ]); $consumer->responses = [ @@ -499,4 +504,20 @@ public function testRetryAfterCapIsRespected(): void self::assertCount(1, $consumer->sleepCalls); self::assertSame(300000 * 1000, $consumer->sleepCalls[0]); } + + public function testRetryAfterCapDefaultsToSixtySeconds(): void + { + // The shipped default, rather than an override: a large Retry-After is + // clamped to the cap, well inside the 5 minute budget. + $consumer = new MockLibCurl('test-secret', ['retry_count' => 3]); + + $consumer->responses = [ + [503, ['retry-after' => '600'], 'Service Unavailable', ''], + [200, [], '{"success":true}', ''], + ]; + + self::assertTrue($consumer->flushBatch(makeTestMessages())); + self::assertCount(1, $consumer->sleepCalls); + self::assertSame(60 * 1000 * 1000, $consumer->sleepCalls[0]); + } } From 6fe7e6fb0b9e1c6ba41336feb51e4928c323c4fb Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 21:19:04 -0400 Subject: [PATCH 04/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. --- lib/Consumer/QueueConsumer.php | 9 ++++----- test/ConsumerLibCurlTest.php | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index a38bbb7..74ea5a1 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -22,11 +22,10 @@ abstract class QueueConsumer extends Consumer protected int $max_total_backoff_duration_ms = 43200000; // 12 hours /** - * Five minutes, in line with the counted-backoff path's ~4 minute worst case. - * This was 12 hours, meant as a backstop a retry count would stop us reaching — - * but rate-limited attempts are deliberately uncounted, so it was the only limit - * on that path. This consumer retries inline on the caller's thread, so that - * budget is time a web request spends blocked. + * Rate-limited attempts are deliberately uncounted, so this duration is the only + * thing bounding them. It matters more here than in a client with a background + * worker: LibCurl retries inline on the caller's thread, so this is time a web + * request spends blocked and an FPM worker spends occupied. */ protected int $max_rate_limit_duration_ms = 300000; // 5 minutes diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 8cd960c..069dd2e 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -243,8 +243,8 @@ public function testRetryAfterOn429DoesNotDecrementRetries(): void /** * retry_count of N grants exactly N counted-backoff retries. * - * The budget used to be decremented before the exhaustion check, so N performed - * N-1 and a retry_count of 1 performed none — indistinguishable from 0. + * Decrementing before the exhaustion check spends one retry on the check itself, + * which yields N-1 and makes a retry_count of 1 indistinguishable from 0. */ public function testRetryCountGrantsExactlyThatManyRetries(): void { @@ -306,9 +306,8 @@ public function testRateLimitSleepNeverOvershootsTheBudget(): void public function testNegativeBudgetOptionsKeepTheDefault(): void { - // A negative value used to be cast straight in, which silently disabled - // retrying: retriesRemaining started below zero and the duration budget - // was already exceeded on the first check. + // A negative cast straight in disables retrying silently: retriesRemaining + // starts below zero and the duration budget is exceeded on its first check. $consumer = new MockLibCurl('test-secret', [ 'retry_count' => -5, 'max_total_backoff_duration' => -1, From 2ac799b91c1338bec71f0f2252365f8504417515 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 24 Sep 2026 09:54:12 -0400 Subject: [PATCH 05/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 +- lib/Consumer/QueueConsumer.php | 8 +++++--- test/ConsumerLibCurlTest.php | 11 ++++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 94db9ec..88e22b4 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -22,7 +22,7 @@ any one retry sleeps. Lower `retry_count` to restore a shorter schedule. * Uploads are retried on 408, 410, 429, 460, and 5xx except 501, 505 and 511. * 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, malformed values are ignored, and the value is capped at `rate_limit_retry_after_cap`. * 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 limited by `retry_count` and by `max_total_backoff_duration` as an upper bound. - * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 60). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. + * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. * Exhausting the rate-limit budget is logged regardless of the `debug` setting. diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 74ea5a1..a008db1 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -30,10 +30,12 @@ abstract class QueueConsumer extends Consumer protected int $max_rate_limit_duration_ms = 300000; // 5 minutes /** - * Kept well below max_rate_limit_duration so the budget buys several attempts - * rather than one long sleep; at the old 300s a single sleep consumed it. + * 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. */ - protected int $rate_limit_retry_after_cap_s = 60; + protected int $rate_limit_retry_after_cap_s = 300; protected int $retry_count = 10; // max retries protected string $host = ''; protected bool $compress_request = false; diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 069dd2e..d595a6d 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -504,19 +504,20 @@ public function testRetryAfterCapIsRespected(): void self::assertSame(300000 * 1000, $consumer->sleepCalls[0]); } - public function testRetryAfterCapDefaultsToSixtySeconds(): void + public function testRetryAfterIsHonouredWhenItFitsTheBudget(): void { - // The shipped default, rather than an override: a large Retry-After is - // clamped to the cap, well inside the 5 minute budget. + // Waiting less than asked sends more requests at a server already + // rate-limiting us, so a value inside the cap and the budget is used + // as given rather than shortened. $consumer = new MockLibCurl('test-secret', ['retry_count' => 3]); $consumer->responses = [ - [503, ['retry-after' => '600'], 'Service Unavailable', ''], + [503, ['retry-after' => '120'], 'Service Unavailable', ''], [200, [], '{"success":true}', ''], ]; self::assertTrue($consumer->flushBatch(makeTestMessages())); self::assertCount(1, $consumer->sleepCalls); - self::assertSame(60 * 1000 * 1000, $consumer->sleepCalls[0]); + self::assertSame(120 * 1000 * 1000, $consumer->sleepCalls[0]); } } From ee118ca4b002f1066ae20834c5ee8aaa97cf031d Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 24 Sep 2026 10:14:51 -0400 Subject: [PATCH 06/10] Default curl_timeout to 300 seconds instead of no limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CURLOPT_TIMEOUT of 0 means curl waits indefinitely, so a connection that stalled after the handshake blocked the caller past every retry budget the rest of this change set added. 300s is well clear of any legitimate upload: batches are capped at 500KB. Also note in the changelog how long a flush can block, and that the file consumer plus send.php is the non-blocking alternative — neither is documented in this repo. --- HISTORY.md | 2 ++ lib/Consumer/QueueConsumer.php | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/HISTORY.md b/HISTORY.md index 88e22b4..55f012b 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -25,11 +25,13 @@ any one retry sleeps. Lower `retry_count` to restore a shorter schedule. * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. * Exhausting the rate-limit budget is logged regardless of the `debug` setting. + * A flush blocks for at most `max_rate_limit_duration` or `max_total_backoff_duration`, plus the `curl_timeout` of the request in flight when the budget runs out. Applications that cannot block for that long can set `consumer` to `file`, which appends events to a log file and makes no network call; the bundled `send.php` delivers that file separately. ### Other changes * `X-Retry-Count` is sent on retries by the LibCurl and Socket consumers, 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: a redirect curl has already declined to follow will not succeed on one. The Segment endpoint does not redirect, so this affects only custom `host` values. + * `curl_timeout` now defaults to 300 seconds rather than 0, which meant no limit: a stalled upload could otherwise block the caller indefinitely. Set `curl_timeout` explicitly to restore the old behaviour or to choose a different bound. 3.8.2 / 2026-03-11 ================== diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index a008db1..989e049 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -40,7 +40,13 @@ abstract class QueueConsumer extends Consumer protected string $host = ''; protected bool $compress_request = false; protected int $flush_interval_in_mills = 10000; //frequency in milliseconds to send data, default 10 - protected int $curl_timeout = 0; // by default this is infinite + + /** + * Bounds one request. A value of 0 means no limit at all, which lets a connection + * that stalls after the handshake hold the caller's thread until the peer or the + * network gives up, outliving every retry budget above it. + */ + protected int $curl_timeout = 300; protected int $curl_connecttimeout = 300; /** From 33de169606292082bdfa2f281cb3ee8e15f726a8 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 24 Sep 2026 10:22:55 -0400 Subject: [PATCH 07/10] Link the file consumer doc from the changelog; drop a duplicate heading The file consumer and send.php are documented after all, so point at that page rather than restating it here. The 3.8.2 heading appeared twice, the first one empty. --- HISTORY.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 55f012b..cb0bc9b 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -25,7 +25,7 @@ any one retry sleeps. Lower `retry_count` to restore a shorter schedule. * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. * Exhausting the rate-limit budget is logged regardless of the `debug` setting. - * A flush blocks for at most `max_rate_limit_duration` or `max_total_backoff_duration`, plus the `curl_timeout` of the request in flight when the budget runs out. Applications that cannot block for that long can set `consumer` to `file`, which appends events to a log file and makes no network call; the bundled `send.php` delivers that file separately. + * A flush blocks for at most `max_rate_limit_duration` or `max_total_backoff_duration`, plus the `curl_timeout` of the request in flight when the budget runs out. Applications that cannot block for that long can use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events to a log file with no network call and uploads them out of band. ### Other changes @@ -33,11 +33,6 @@ any one retry sleeps. Lower `retry_count` to restore a shorter schedule. * 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: a redirect curl has already declined to follow will not succeed on one. The Segment endpoint does not redirect, so this affects only custom `host` values. * `curl_timeout` now defaults to 300 seconds rather than 0, which meant no limit: a stalled upload could otherwise block the caller indefinitely. Set `curl_timeout` explicitly to restore the old behaviour or to choose a different bound. -3.8.2 / 2026-03-11 -================== - - - 3.8.2 / 2026-03-11 ================== From 9d05ff8177a7dc0ab11af0b3fc856bdc05ba6c6e Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 15:28:40 -0400 Subject: [PATCH 08/10] Clamp the backoff wait, reject a zero cap, and tell LibCurl users what changed The clamp added for the rate-limit path left the counted-backoff path with the same defect: max_total_backoff_duration was tested and then a full backoff interval slept on top, overshooting by up to the 60s ceiling. Both budgets now compute their remaining time from one clock reading and clamp the wait to it. rate_limit_retry_after_cap accepted 0. That clamps every rate-limit wait to zero, and that path deliberately does not consume a retry, so the consumer posted back-to-back at RTT rate for the whole budget at a server already rate-limiting it. It now requires a positive integer. retry_count and curl_timeout still accept 0, where it means something. The release notes had an upgrade note for the Socket consumer and none for LibCurl, which is the default and changed the same way: 3.8.2's LibCurl was also a fixed seven retries over ~13 seconds ($backoff = 100 doubling while < maximum_backoff_duration), and it now runs ~4 minutes of waits inline on the calling thread. The note now covers both and says which consumer caps an individual wait with what -- Socket honours maximum_backoff_duration, LibCurl has a fixed 60s ceiling and never reads it. The comment in Socket.php claiming the two share "the same budgets" is corrected to say which are shared. Also documents that track() can now return false where it previously returned true, since flushBatch reports 4xx and retry-exhausted uploads as failures and flush() stops at the first failing batch. The blocking-time bullet described a bound per flush; it holds per batch, and flush() sends as many batches as the queue holds with flush_interval between them. 86 tests, same 2 pre-existing failures (both PHP 8.5 curl_close deprecation artifacts; CI runs 8.1/8.2). phpcs and parallel-lint clean. --- HISTORY.md | 37 +++++++++++++++++++++++++--------- lib/Consumer/LibCurl.php | 15 +++++++++++--- lib/Consumer/QueueConsumer.php | 21 ++++++++++++++++++- lib/Consumer/Socket.php | 5 ++++- test/ConsumerLibCurlTest.php | 14 +++++++++++++ 5 files changed, 78 insertions(+), 14 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index cb0bc9b..33f8dcd 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,14 +8,33 @@ Segment passes through a proxy, gateway or WAF that allowlists request headers, add it before upgrading or retried uploads will be rejected. The `Authorization` header is unchanged. -### Upgrade note: Socket consumer retry behaviour - -The Socket consumer previously gave up after a fixed seven retries over roughly -13 seconds. It now honours `retry_count` and `max_total_backoff_duration`, and -backs off from 500ms rather than 100ms, so a failing upload is retried for -longer than before. `maximum_backoff_duration` now caps each individual wait -rather than ending the retry loop; its 10 second default still bounds how long -any one retry sleeps. Lower `retry_count` to restore a shorter schedule. +### Upgrade note: retries take longer than they used to + +Both the LibCurl and Socket consumers previously gave up after a fixed seven +retries over roughly 13 seconds. Both now honour `retry_count` (default 10) and +back off from 500ms rather than 100ms, so a failing upload is retried for +considerably longer than before. + +This matters most for the default LibCurl consumer, which retries inline on the +calling thread: with the default `retry_count` a persistently failing upload +spends around four minutes in waits, plus up to `curl_timeout` per attempt, +before giving up. On a web request that is a worker held for the duration. Lower +`retry_count` to restore a shorter schedule, or use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), +which records events without making a network call. + +The two consumers cap an individual wait differently: LibCurl caps each wait at +60 seconds, while Socket caps it at `maximum_backoff_duration`, whose default is +10 seconds. + +### Upgrade note: `track()` return value + +`track()` and the other message methods return the result of a flush when the +queue reaches `flush_at`. Previously the LibCurl consumer reported success for +any response it had finished with, including a 4xx and a retry-exhausted upload, +so that flush almost always returned `true`. It now returns `false` when the +batch was not delivered, and `flush()` stops at the first failing batch rather +than continuing through the queue. Code branching on the return value of +`track()` will see failures it did not see before. ### Retry handling @@ -25,7 +44,7 @@ any one retry sleeps. Lower `retry_count` to restore a shorter schedule. * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. * Exhausting the rate-limit budget is logged regardless of the `debug` setting. - * A flush blocks for at most `max_rate_limit_duration` or `max_total_backoff_duration`, plus the `curl_timeout` of the request in flight when the budget runs out. Applications that cannot block for that long can use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events to a log file with no network call and uploads them out of band. + * These budgets bound one batch: it is retried for up to `max_rate_limit_duration` on responses carrying `Retry-After` and, independently, up to `max_total_backoff_duration` on those without, so a response stream that mixes the two spends both — plus the `curl_timeout` of the request in flight when a budget runs out. A single `flush()` sends as many batches as the queue holds, waiting `flush_interval` between them, so it can take considerably longer than any one batch's budget. Applications that cannot block for that long can use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events to a log file with no network call and uploads them out of band. ### Other changes diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 7d5ab82..a9447e5 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -129,13 +129,22 @@ public function flushBatch(array $messages): bool return false; } $retriesRemaining--; + // One reading serves the budget test and the clamp below, as on the + // rate-limit path above. + $backoffNow = hrtime(true); if ($backoffStartTime === null) { - $backoffStartTime = hrtime(true); + $backoffStartTime = $backoffNow; } - if ((hrtime(true) - $backoffStartTime) / 1e6 >= $this->max_total_backoff_duration_ms) { + $backoffRemainingMs = (int)( + $this->max_total_backoff_duration_ms - ($backoffNow - $backoffStartTime) / 1e6 + ); + if ($backoffRemainingMs <= 0) { return false; } - $this->sleepBeforeRetry($backoffMs, false); + // Clamped for the same reason as the rate-limit path: the budget is + // checked before the wait, so an unclamped sleep overshoots it by up to + // the backoff ceiling. + $this->sleepBeforeRetry(min($backoffMs, $backoffRemainingMs), false); $backoffMs = min($backoffMs * 2, $backoffCapMs); } } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 989e049..1f45e93 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -128,8 +128,13 @@ public function __construct(string $secret, array $options = []) } } + // Positive, not merely non-negative. A cap of 0 clamps every wait to 0, and + // the rate-limit path does not consume a retry, so the client would post + // back-to-back at RTT rate for the whole budget against a server that is + // already rate-limiting it. 0 is meaningful for retry_count and curl_timeout, + // but there is no sensible reading of "cap the wait at nothing". if (isset($options['rate_limit_retry_after_cap'])) { - if ($this->isNonNegativeInt($options['rate_limit_retry_after_cap'], 'rate_limit_retry_after_cap')) { + if ($this->isPositiveInt($options['rate_limit_retry_after_cap'], 'rate_limit_retry_after_cap')) { $this->rate_limit_retry_after_cap_s = (int)$options['rate_limit_retry_after_cap']; } } @@ -190,6 +195,20 @@ public function flush(): bool * accepted: analytics-python validates these the same way, and retry_count 0 * meaning "do not retry" is deliberate there and in analytics-ruby. */ + protected function isPositiveInt($value, string $name): bool + { + if (!is_numeric($value) || (int)$value < 1) { + error_log(sprintf( + '[Analytics][%s] %s must be a positive integer; keeping the default', + $this->type, + $name + )); + return false; + } + + return true; + } + protected function isNonNegativeInt($value, string $name): bool { if (!is_numeric($value) || (int)$value < 0) { diff --git a/lib/Consumer/Socket.php b/lib/Consumer/Socket.php index 8bf94fb..90ae118 100644 --- a/lib/Consumer/Socket.php +++ b/lib/Consumer/Socket.php @@ -149,7 +149,10 @@ private function createBody(string $host, string $content, int $attempt = 1) * - X-Retry-Count: Supported. * - Backoff: Exponential from 500ms, each wait capped at * maximum_backoff_duration, bounded by retry_count and - * max_total_backoff_duration — the same budgets the LibCurl consumer uses. + * max_total_backoff_duration. retry_count and max_total_backoff_duration are + * shared with LibCurl; the per-wait cap is not. LibCurl caps each wait at a + * fixed 60s and does not read maximum_backoff_duration at all, so the same + * configuration produces different schedules on the two consumers. * - max_rate_limit_duration: not applicable, since there is no Retry-After * path here for it to bound. * diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index d595a6d..9eb0c78 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -323,6 +323,20 @@ public function testNegativeBudgetOptionsKeepTheDefault(): void self::assertSame(43200000, $read('max_total_backoff_duration_ms'), 'default 12h budget'); } + public function testZeroRetryAfterCapKeepsTheDefault(): void + { + // Unlike retry_count, 0 is not a meaningful cap. It clamps every rate-limit + // wait to zero, and that path does not consume a retry, so the consumer would + // post back-to-back at RTT rate for the whole budget at a server that is + // already rate-limiting it. + $consumer = new MockLibCurl('test-secret', ['rate_limit_retry_after_cap' => 0]); + + $ref = new \ReflectionProperty(QueueConsumer::class, 'rate_limit_retry_after_cap_s'); + $ref->setAccessible(true); + + self::assertSame(300, $ref->getValue($consumer), 'a cap of 0 must not be accepted'); + } + public function testZeroRetryCountIsAcceptedRatherThanTreatedAsInvalid(): void { // retry_count 0 means "do not retry" and is deliberate in analytics-python From b8385912271e268b7a64ac13ae147282f87c6d5e Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 17:17:44 -0400 Subject: [PATCH 09/10] End an episode rather than resume inside the window, and correct four claims The remaining-budget clamp becomes a drop, on both paths. Shortening a Retry-After to fit resumes inside the window the server named -- one it has already said it will not serve -- and the budget is spent by then, so that attempt would be the last either way. It matters more here than elsewhere: this consumer blocks the calling thread, so the shortened wait is paid for in held FPM workers as well as in a refused request. That also removes the int truncation review flagged. The remaining budget was cast to int, so a sub-millisecond remainder became 0, min() returned 0, and the consumer slept not at all and re-POSTed at round-trip rate without consuming a retry. Both budgets now compare in floats and only the sleep is cast. Three corrections to the release notes: "Seven retries over roughly 13 seconds" was right for Socket and wrong for LibCurl. Verified against the 3.8.2 tag: Socket checks its ceiling after the failure and gets 8 attempts, LibCurl checks `$backoff < maximum_backoff_duration` before each attempt and gets 7. Same 13 seconds either way. The note said only negatives are ignored; a rate_limit_retry_after_cap of 0 is now rejected too, and a reader had no way to predict that. max_rate_limit_duration's default said 300 with no explanation, while the other server libraries say 30 minutes. The reason -- inline retries hold an FPM worker -- lived only in a code comment, so a reader comparing changelogs would see php as an oversight rather than a deliberate divergence. The new isPositiveInt had been inserted between the existing docblock and isNonNegativeInt, leaving a paragraph reading "Zero is accepted" attached to the method whose purpose is rejecting zero. Both now carry their own. 87 tests, same 2 pre-existing failures (PHP 8.5 curl_close deprecation; CI runs 8.1/8.2). phpcs and parallel-lint clean. --- HISTORY.md | 7 +++--- lib/Consumer/LibCurl.php | 43 ++++++++++++++++++++-------------- lib/Consumer/QueueConsumer.php | 13 ++++++---- test/ConsumerLibCurlTest.php | 39 ++++++++++++++++++------------ test/MockLibCurl.php | 5 ++++ 5 files changed, 68 insertions(+), 39 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 33f8dcd..32a679a 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -10,8 +10,9 @@ header is unchanged. ### Upgrade note: retries take longer than they used to -Both the LibCurl and Socket consumers previously gave up after a fixed seven -retries over roughly 13 seconds. Both now honour `retry_count` (default 10) and +Both the LibCurl and Socket consumers previously gave up after roughly 13 +seconds of waiting — seven retries for Socket, six for LibCurl, which checked +its backoff ceiling before each attempt rather than after. Both now honour `retry_count` (default 10) and back off from 500ms rather than 100ms, so a failing upload is retried for considerably longer than before. @@ -41,7 +42,7 @@ than continuing through the queue. Code branching on the return value of * Uploads are retried on 408, 410, 429, 460, and 5xx except 501, 505 and 511. * 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, malformed values are ignored, and the value is capped at `rate_limit_retry_after_cap`. * 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 limited by `retry_count` and by `max_total_backoff_duration` as an upper bound. - * New options, all in seconds: `max_rate_limit_duration` (default 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept. A `retry_count` of 0 means do not retry. + * New options, all in seconds: `max_rate_limit_duration` (default 300 — deliberately shorter than in Segment's other server libraries, because the LibCurl consumer retries inline on the calling thread, so this budget is time a web request spends blocked and a PHP-FPM worker spends occupied), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept, as is a `rate_limit_retry_after_cap` of 0 — a cap of zero would clamp every wait to nothing and, since that path does not consume a retry, post continuously for the whole budget. A `retry_count` of 0 does mean do not retry, and `curl_timeout` of 0 still means no limit. * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. * Exhausting the rate-limit budget is logged regardless of the `debug` setting. * These budgets bound one batch: it is retried for up to `max_rate_limit_duration` on responses carrying `Retry-After` and, independently, up to `max_total_backoff_duration` on those without, so a response stream that mixes the two spends both — plus the `curl_timeout` of the request in flight when a budget runs out. A single `flush()` sends as many batches as the queue holds, waiting `flush_interval` between them, so it can take considerably longer than any one batch's budget. Applications that cannot block for that long can use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events to a log file with no network call and uploads them out of band. diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index a9447e5..f3ad3b7 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -108,16 +108,24 @@ public function flushBatch(array $messages): bool )); return false; } - // Clamped to the remaining budget as well as the cap: the elapsed check - // above runs before the wait, so without this a check passing just inside - // the budget would sleep a full Retry-After on top and overshoot it. - $remainingMs = (int)($this->max_rate_limit_duration_ms - $elapsedMs); - $sleepMs = min( - $retryAfterS * 1000, - $this->rate_limit_retry_after_cap_s * 1000, - $remainingMs - ); - $this->sleepBeforeRetry($sleepMs, true); + // Capped, then required to fit. Shortening the wait would resume inside + // the window the server named -- one it has already said it will not + // serve -- and the budget is spent by then, so that attempt would be the + // last either way. Compared in floats: an int cast here truncates a + // sub-millisecond remainder to 0, which would sleep not at all and + // re-POST at round-trip rate without consuming a retry. + $remainingMs = $this->max_rate_limit_duration_ms - $elapsedMs; + $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); + if ($sleepMs > $remainingMs) { + error_log(sprintf( + '[Analytics][%s] Retry-After of %ds does not fit the remaining ' + . 'rate-limit budget; dropping batch', + $this->type, + (int)($sleepMs / 1000) + )); + return false; + } + $this->sleepBeforeRetry((int)$sleepMs, true); continue; // Do NOT decrement retriesRemaining } @@ -135,16 +143,17 @@ public function flushBatch(array $messages): bool if ($backoffStartTime === null) { $backoffStartTime = $backoffNow; } - $backoffRemainingMs = (int)( - $this->max_total_backoff_duration_ms - ($backoffNow - $backoffStartTime) / 1e6 - ); + $backoffRemainingMs = + $this->max_total_backoff_duration_ms - ($backoffNow - $backoffStartTime) / 1e6; if ($backoffRemainingMs <= 0) { return false; } - // Clamped for the same reason as the rate-limit path: the budget is - // checked before the wait, so an unclamped sleep overshoots it by up to - // the backoff ceiling. - $this->sleepBeforeRetry(min($backoffMs, $backoffRemainingMs), false); + // Same rule as the rate-limit path, for the simpler reason that a backoff + // outlasting the budget is a wait whose attempt can never run. + if ($backoffMs > $backoffRemainingMs) { + return false; + } + $this->sleepBeforeRetry($backoffMs, false); $backoffMs = min($backoffMs * 2, $backoffCapMs); } } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 1f45e93..7e42e1b 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -189,11 +189,9 @@ public function flush(): bool } /** - * Whether an option value is usable as a count or duration. + * Whether an option value is usable where zero is not meaningful. * - * Logs and returns false otherwise, so the caller keeps the default. Zero is - * accepted: analytics-python validates these the same way, and retry_count 0 - * meaning "do not retry" is deliberate there and in analytics-ruby. + * Logs and returns false otherwise, so the caller keeps the default. */ protected function isPositiveInt($value, string $name): bool { @@ -209,6 +207,13 @@ protected function isPositiveInt($value, string $name): bool return true; } + /** + * Whether an option value is usable as a count or duration. + * + * Logs and returns false otherwise, so the caller keeps the default. Zero is + * accepted: analytics-python validates these the same way, and retry_count 0 + * meaning "do not retry" is deliberate there and in analytics-ruby. + */ protected function isNonNegativeInt($value, string $name): bool { if (!is_numeric($value) || (int)$value < 0) { diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 9eb0c78..01a5f4b 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -275,33 +275,42 @@ public function testRetryCountOfZeroGrantsNoRetries(): void self::assertCount(0, $consumer->sleepCalls); } - public function testRateLimitSleepNeverOvershootsTheBudget(): void + public function testARateLimitWaitThatCannotFitTheBudgetDropsWithoutWaiting(): void { - // The elapsed check runs before the wait, so without clamping a check - // passing just inside the budget sleeps a full Retry-After on top. At a - // 5 minute budget that doubles the bound rather than rounding it, and - // this consumer blocks the caller for the whole of it. + // Shortening the wait would resume inside the window the server named -- + // one it has already said it will not serve -- and the budget is spent by + // then, so that attempt would be the last either way. This consumer blocks + // the caller for the whole wait, so the shortened one is paid for twice. $consumer = new MockLibCurl('test-secret', [ 'max_rate_limit_duration' => 1, // 1 second of budget 'retry_count' => 5, ]); $consumer->responses = [ - [429, ['retry-after' => '60'], 'Too Many Requests', ''], [429, ['retry-after' => '60'], 'Too Many Requests', ''], [200, [], '{"success":true}', ''], ]; - $consumer->flushBatch(makeTestMessages()); + self::assertFalse($consumer->flushBatch(makeTestMessages())); + self::assertEmpty($consumer->sleepCalls, 'should not have waited at all'); + self::assertSame(1, $consumer->requestCount, 'should not have made a second request'); + } - self::assertNotEmpty($consumer->sleepCalls); - foreach ($consumer->sleepCalls as $micros) { - self::assertLessThanOrEqual( - 1_000_000, - $micros, - 'slept ' . $micros . 'us with at most 1s of budget left' - ); - } + public function testARateLimitWaitThatFitsIsHonouredInFull(): void + { + // "Never shorten" must not become "never wait". + $consumer = new MockLibCurl('test-secret', [ + 'max_rate_limit_duration' => 300, + 'retry_count' => 5, + ]); + + $consumer->responses = [ + [429, ['retry-after' => '2'], 'Too Many Requests', ''], + [200, [], '{"success":true}', ''], + ]; + + self::assertTrue($consumer->flushBatch(makeTestMessages())); + self::assertSame([2_000_000], $consumer->sleepCalls); } public function testNegativeBudgetOptionsKeepTheDefault(): void diff --git a/test/MockLibCurl.php b/test/MockLibCurl.php index 28c7b3d..f026c01 100644 --- a/test/MockLibCurl.php +++ b/test/MockLibCurl.php @@ -24,6 +24,9 @@ class MockLibCurl extends LibCurl /** @var int how many counted-backoff waits were performed */ public int $backoffSleeps = 0; + /** @var int how many HTTP attempts were made */ + public int $requestCount = 0; + public function __construct(string $secret, array $options = []) { parent::__construct($secret, $options); @@ -31,6 +34,8 @@ public function __construct(string $secret, array $options = []) protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array { + $this->requestCount++; + if (empty($this->responses)) { return [200, [], '{"success":true}', '', 0]; } From 5487c3f4031596d92449f3d6e0b1688c038ebe0b Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 17:34:09 -0400 Subject: [PATCH 10/10] Take the curl_timeout default change back out of this PR Unrelated to retry budgets, and the case for it was weaker than it looked when it went in. The figure it was justified against was wrong: a flush was described as bounded by one budget plus one curl_timeout, when the budgets are per batch, are spent independently by a mixed response stream, and a flush sends as many batches as the queue holds. At the shipped defaults one batch is ~243s of backoff waits plus ~300s of rate-limit waits plus eleven attempts, so a per-request timeout of 300 makes the attempts term 3300s rather than bounding anything usefully -- about 64 minutes for a single batch. A per-request timeout is the wrong shape of bound for that; a deadline checked across the whole of flushBatch is. That is worth doing on its own, where it can be reasoned about and tested as the thing it is. Reverting also restores the status quo for anyone already relying on it. 0 means no libcurl timeout, but curl_connecttimeout still bounds the connect phase, the kernel's TCP retransmission timeout still kills a dead peer, and PHP-FPM's request_terminate_timeout still bounds the worker -- so the unbounded case is narrower than "hangs forever", and anyone it reached has long since set the option or configured a backstop. 87 tests, same 2 pre-existing failures. phpcs and parallel-lint clean. --- HISTORY.md | 5 ++--- lib/Consumer/QueueConsumer.php | 7 +------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 32a679a..e3e1f61 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -18,7 +18,7 @@ considerably longer than before. This matters most for the default LibCurl consumer, which retries inline on the calling thread: with the default `retry_count` a persistently failing upload -spends around four minutes in waits, plus up to `curl_timeout` per attempt, +spends around four minutes in waits, plus however long each attempt takes, before giving up. On a web request that is a worker held for the duration. Lower `retry_count` to restore a shorter schedule, or use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events without making a network call. @@ -45,13 +45,12 @@ than continuing through the queue. Code branching on the return value of * New options, all in seconds: `max_rate_limit_duration` (default 300 — deliberately shorter than in Segment's other server libraries, because the LibCurl consumer retries inline on the calling thread, so this budget is time a web request spends blocked and a PHP-FPM worker spends occupied), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). Negative values are ignored, logged, and the default kept, as is a `rate_limit_retry_after_cap` of 0 — a cap of zero would clamp every wait to nothing and, since that path does not consume a retry, post continuously for the whole budget. A `retry_count` of 0 does mean do not retry, and `curl_timeout` of 0 still means no limit. * `Retry-After` is not read by the Socket consumer, which uses exponential backoff for every retryable response. Use the default LibCurl consumer if you need it. * Exhausting the rate-limit budget is logged regardless of the `debug` setting. - * These budgets bound one batch: it is retried for up to `max_rate_limit_duration` on responses carrying `Retry-After` and, independently, up to `max_total_backoff_duration` on those without, so a response stream that mixes the two spends both — plus the `curl_timeout` of the request in flight when a budget runs out. A single `flush()` sends as many batches as the queue holds, waiting `flush_interval` between them, so it can take considerably longer than any one batch's budget. Applications that cannot block for that long can use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events to a log file with no network call and uploads them out of band. + * These budgets bound one batch: it is retried for up to `max_rate_limit_duration` on responses carrying `Retry-After` and, independently, up to `max_total_backoff_duration` on those without, so a response stream that mixes the two spends both — plus however long the request in flight takes, which `curl_timeout` bounds only if it has been set — it defaults to no limit. A single `flush()` sends as many batches as the queue holds, waiting `flush_interval` between them, so it can take considerably longer than any one batch's budget. Applications that cannot block for that long can use the [file consumer](https://www.twilio.com/docs/segment/connections/sources/catalog/libraries/server/php#file-consumer), which records events to a log file with no network call and uploads them out of band. ### Other changes * `X-Retry-Count` is sent on retries by the LibCurl and Socket consumers, 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: a redirect curl has already declined to follow will not succeed on one. The Segment endpoint does not redirect, so this affects only custom `host` values. - * `curl_timeout` now defaults to 300 seconds rather than 0, which meant no limit: a stalled upload could otherwise block the caller indefinitely. Set `curl_timeout` explicitly to restore the old behaviour or to choose a different bound. 3.8.2 / 2026-03-11 ================== diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 7e42e1b..576ee34 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -41,12 +41,7 @@ abstract class QueueConsumer extends Consumer protected bool $compress_request = false; protected int $flush_interval_in_mills = 10000; //frequency in milliseconds to send data, default 10 - /** - * Bounds one request. A value of 0 means no limit at all, which lets a connection - * that stalls after the handshake hold the caller's thread until the peer or the - * network gives up, outliving every retry budget above it. - */ - protected int $curl_timeout = 300; + protected int $curl_timeout = 0; // by default this is infinite protected int $curl_connecttimeout = 300; /**