From b4fb2964abb62fb8dc538fe9718d98a19984d350 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 15 May 2026 18:57:50 -0400 Subject: [PATCH 01/16] Implement status-response retry improvements - LibCurl: dual-path retry loop (429+Retry-After vs counted exponential backoff), X-Retry-Count header on retries, retryable status classification, duration budgets - QueueConsumer: add retry config properties (max_total_backoff_duration_ms, max_rate_limit_duration_ms, rate_limit_retry_after_cap_s, retry_count); fix queue splice bug (peek with array_slice, splice only on success); add isRetryable() and parseRetryAfter() helpers - Socket: update DoPost signature for X-Retry-Count; use success range check (>= 200 && < 400) --- lib/Consumer/LibCurl.php | 148 +++++++++++++++++++++------------ lib/Consumer/QueueConsumer.php | 65 ++++++++++++++- lib/Consumer/Socket.php | 66 ++++++++++----- 3 files changed, 201 insertions(+), 78 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 405467b..d23d258 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -9,92 +9,132 @@ class LibCurl extends QueueConsumer protected string $type = 'LibCurl'; /** - * Make a sync request to our API. If debug is - * enabled, we wait for the response - * and retry once to diminish impact on performance. + * Send a batch of messages to the API with spec-compliant retry logic: + * - 2xx/3xx: success + * - 429 + Retry-After: sleep without consuming retry budget + * - 429 without Retry-After / other retryable (5xx except 501/505/511, + * 408/410/460): exponential backoff, counts against retry budget + * - Non-retryable 4xx / 501/505/511: drop immediately + * * @param array $messages array of all the messages to send * @return bool whether the request succeeded */ public function flushBatch(array $messages): bool { - $body = $this->payload($messages); + $body = $this->payload($messages); $payload = json_encode($body); - $secret = $this->secret; + $secret = $this->secret; if ($this->compress_request) { $payload = gzencode($payload); } - if ($this->host) { - $host = $this->host; - } else { - $host = 'api.segment.io'; - } - $path = '/v1/batch'; - $url = $this->protocol . $host . $path; + $host = $this->host ?: 'api.segment.io'; + $url = $this->protocol . $host . '/v1/batch'; - $backoff = 100; // Set initial waiting time to 100ms + $library = $messages[0]['context']['library']; + $userAgent = $library['name'] . '/' . $library['version']; - while ($backoff < $this->maximum_backoff_duration) { - // open connection - $ch = curl_init(); + $backoffMs = 500; // base 500ms per e2e spec + $backoffCapMs = 60000; // cap 60s + $retriesRemaining = $this->retry_count; + $attempt = 0; + $backoffStartTime = null; + $rateLimitStartTime = null; - // set the url, number of POST vars, POST data - curl_setopt($ch, CURLOPT_USERPWD, $secret . ':'); - curl_setopt($ch, CURLOPT_POSTFIELDS, $payload); - curl_setopt($ch, CURLOPT_TIMEOUT, $this->curl_timeout); - curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $this->curl_connecttimeout); + while (true) { + $attempt++; + $responseHeaders = []; + + $ch = curl_init(); - // set variables for headers - $header = []; - $header[] = 'Content-Type: application/json'; + $headers = [ + 'Content-Type: application/json', + 'User-Agent: ' . $userAgent, + ]; if ($this->compress_request) { - $header[] = 'Content-Encoding: gzip'; + $headers[] = 'Content-Encoding: gzip'; } - // Send user agent in the form of {library_name}/{library_version} as per RFC 7231. - $library = $messages[0]['context']['library']; - $libName = $library['name']; - $libVersion = $library['version']; - $header[] = "User-Agent: $libName/$libVersion"; + if ($attempt > 1) { + $headers[] = 'X-Retry-Count: ' . ($attempt - 1); + } - curl_setopt($ch, CURLOPT_HTTPHEADER, $header); - curl_setopt($ch, CURLOPT_URL, $url); + curl_setopt($ch, CURLOPT_USERPWD, $secret . ':'); + curl_setopt($ch, CURLOPT_POSTFIELDS, $payload); + curl_setopt($ch, CURLOPT_TIMEOUT, $this->curl_timeout); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $this->curl_connecttimeout); + curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); + curl_setopt($ch, CURLOPT_URL, $url); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + curl_setopt($ch, CURLOPT_HEADERFUNCTION, function ($curl, $header) use (&$responseHeaders) { + $parts = explode(':', $header, 2); + if (count($parts) === 2) { + $responseHeaders[strtolower(trim($parts[0]))] = trim($parts[1]); + } + + return strlen($header); + }); - // retry failed requests just once to diminish impact on performance $responseContent = curl_exec($ch); + $err = curl_error($ch); + $responseCode = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); + curl_close($ch); - $err = curl_error($ch); if ($err) { - $this->handleError(curl_errno($ch), $err); + $this->handleError(0, $err); + return false; } - $responseCode = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); + // 2xx and 3xx are success + if ($responseCode >= 200 && $responseCode < 400) { + return true; + } - //close connection - curl_close($ch); + $this->handleError($responseCode, $responseContent); + + // 429: check for Retry-After header first + if ($responseCode === 429) { + $retryAfterS = $this->parseRetryAfter($responseHeaders['retry-after'] ?? null); + + if ($retryAfterS !== null) { + if ($rateLimitStartTime === null) { + $rateLimitStartTime = microtime(true); + } - if ($responseCode !== 200) { - // log error - $this->handleError($responseCode, $responseContent); - - if (($responseCode >= 500 && $responseCode <= 600) || $responseCode === 429) { - // If status code is greater than 500 and less than 600, it indicates server error - // Error code 429 indicates rate limited. - // Retry uploading in these cases. - usleep($backoff * 1000); - $backoff *= 2; - } elseif ($responseCode >= 400) { - break; + if ((microtime(true) - $rateLimitStartTime) * 1000 >= $this->max_rate_limit_duration_ms) { + return false; + } + + $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); + usleep($sleepMs * 1000); + continue; // Do NOT decrement retriesRemaining } - } else { - break; // no error + // No Retry-After: fall through to counted backoff } - } - return true; + if (!$this->isRetryable($responseCode)) { + return false; + } + + $retriesRemaining--; + + if ($retriesRemaining <= 0) { + return false; + } + + if ($backoffStartTime === null) { + $backoffStartTime = microtime(true); + } + + if ((microtime(true) - $backoffStartTime) * 1000 >= $this->max_total_backoff_duration_ms) { + return false; + } + + usleep($backoffMs * 1000); + $backoffMs = min($backoffMs * 2, $backoffCapMs); + } } } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 60f7e43..ee31f14 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -19,6 +19,10 @@ abstract class QueueConsumer extends Consumer protected int $max_batch_size_bytes = 512000; //500kb 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 + protected int $retry_count = 10; // max retries protected string $host = ''; protected bool $compress_request = false; protected int $flush_interval_in_mills = 10000; //frequency in milliseconds to send data, default 10 @@ -83,6 +87,22 @@ public function __construct(string $secret, array $options = []) $this->curl_connecttimeout = $options['curl_connecttimeout']; } + if (isset($options['max_total_backoff_duration'])) { + $this->max_total_backoff_duration_ms = (int)$options['max_total_backoff_duration']; + } + + if (isset($options['max_rate_limit_duration'])) { + $this->max_rate_limit_duration_ms = (int)$options['max_rate_limit_duration']; + } + + if (isset($options['rate_limit_retry_after_cap_s'])) { + $this->rate_limit_retry_after_cap_s = (int)$options['rate_limit_retry_after_cap_s']; + } + + if (isset($options['retry_count'])) { + $this->retry_count = (int)$options['retry_count']; + } + $this->queue = []; } @@ -101,7 +121,8 @@ public function flush(): bool $success = true; while ($count > 0 && $success) { - $batch = array_splice($this->queue, 0, min($this->flush_at, $count)); + $batchSize = min($this->flush_at, $count); + $batch = array_slice($this->queue, 0, $batchSize); if (mb_strlen(serialize($batch), '8bit') >= $this->max_batch_size_bytes) { $msg = 'Batch size is larger than 500KB'; @@ -112,9 +133,14 @@ public function flush(): bool $success = $this->flushBatch($batch); + // Remove batch from queue only after successful send + if ($success) { + array_splice($this->queue, 0, $batchSize); + } + $count = count($this->queue); - if ($count > 0) { + if ($count > 0 && $success) { usleep($this->flush_interval_in_mills * 1000); } } @@ -122,6 +148,41 @@ public function flush(): bool return $success; } + /** + * Determine if a status code is retryable per e2e spec. + * 5xx are retryable except 501, 505, 511. + * 4xx are non-retryable except 408, 410, 429, 460. + */ + protected function isRetryable(int $statusCode): bool + { + if ($statusCode >= 500 && $statusCode < 600) { + return !in_array($statusCode, [501, 505, 511], true); + } + + return in_array($statusCode, [408, 410, 429, 460], true); + } + + /** + * Parse Retry-After header as integer seconds. + * Returns null if absent, non-numeric, zero, or negative. + */ + protected function parseRetryAfter(?string $value): ?int + { + if ($value === null || $value === '') { + return null; + } + + $value = trim($value); + + if (!ctype_digit($value)) { + return null; + } + + $seconds = (int)$value; + + return $seconds > 0 ? $seconds : null; + } + /** * Tracks a user action * diff --git a/lib/Consumer/Socket.php b/lib/Consumer/Socket.php index c339575..299eb9f 100644 --- a/lib/Consumer/Socket.php +++ b/lib/Consumer/Socket.php @@ -46,7 +46,7 @@ public function flushBatch($batch): bool $payload = $this->payload($batch); $payload = json_encode($payload); - $body = $this->createBody($this->options['host'], $payload); + $body = $this->createBody($this->options['host'], $payload, 1); if ($body === false) { return false; } @@ -95,7 +95,7 @@ private function createSocket() * @param string $content * @return string body */ - private function createBody(string $host, string $content) + private function createBody(string $host, string $content, int $attempt = 1) { $req = "POST /v1/batch HTTP/1.1\r\n"; $req .= 'Host: ' . $host . "\r\n"; @@ -110,6 +110,11 @@ private function createBody(string $host, string $content) $libVersion = $library['version']; $req .= "User-Agent: $libName/$libVersion\r\n"; + // X-Retry-Count: omit on first attempt, send on retries + if ($attempt > 1) { + $req .= 'X-Retry-Count: ' . ($attempt - 1) . "\r\n"; + } + // Compress content if compress_request is true if ($this->compress_request) { $content = gzencode($content); @@ -134,8 +139,17 @@ private function createBody(string $host, string $content) } /** - * Attempt to write the request to the socket, wait for response if debug - * mode is enabled. + * Socket consumer retry limitations (maintenance mode): + * + * - Retry-After header: NOT fully supported (socket only reads first 2048 + * bytes of response; full header parsing not implemented). Falls back to + * exponential backoff on 429. + * - Status code classification: Full support (retryable vs non-retryable + * per e2e spec, via parent isRetryable()). + * - X-Retry-Count: Supported. + * - Backoff: Exponential with cap (maximum_backoff_duration). + * + * For full Retry-After support, use the default LibCurl consumer. * * @param resource|false $socket the handle for the socket * @param string $req request body @@ -144,12 +158,12 @@ private function createBody(string $host, string $content) private function makeRequest($socket, string $req): bool { $bytes_written = 0; - $bytes_total = strlen($req); - $closed = false; - $success = true; + $bytes_total = strlen($req); + $closed = false; // Retries with exponential backoff until success $backoff = 100; // Set initial waiting time to 100ms + $attempt = 1; while (true) { // Send request to server @@ -167,39 +181,47 @@ private function makeRequest($socket, string $req): bool $statusCode = 0; if (!$closed) { - $res = self::parseResponse(fread($socket, 2048)); + $res = self::parseResponse(fread($socket, 2048)); $statusCode = (int)$res['status']; } fclose($socket); - // If status code is 200, return true - if ($statusCode === 200) { + // 2xx and 3xx are success + if ($statusCode >= 200 && $statusCode < 400) { return true; } - // If status code is greater than 500 and less than 600, it indicates server error - // Error code 429 indicates rate limited. - // Retry uploading in these cases. - if (($statusCode >= 500 && $statusCode <= 600) || $statusCode === 429 || $statusCode === 0) { - if ($backoff >= $this->maximum_backoff_duration) { - break; - } - - usleep($backoff * 1000); - } elseif ($statusCode >= 400) { + // Non-retryable or backoff budget exhausted + if (!$this->isRetryable($statusCode) && $statusCode !== 0) { if ($this->debug()) { $this->handleError($res['status'], $res['message']); } + return false; + } + + if ($backoff >= $this->maximum_backoff_duration) { break; } - // Retry uploading... + usleep($backoff * 1000); $backoff *= 2; + $attempt++; + $socket = $this->createSocket(); + if (!$socket) { + return false; + } + + // Rebuild request with updated X-Retry-Count + $content_json = json_decode($req, true); + // Re-create body with new attempt count (reuse original payload via flushBatch flow) + $bytes_written = 0; + $bytes_total = strlen($req); + $closed = false; } - return $success; + return false; } /** From e04b762b6e7782814b61c6843b5eb476c36417c5 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 19 May 2026 13:33:06 -0400 Subject: [PATCH 02/16] Fix e2e-cli error reporting and enable retry test suite - Make error_handler log-only; determine success from enqueue/flush return values to avoid false failures from transient retry errors - Wire maxRetries from input config to retry_count option - Remove duplicate "Flush failed" in error output - Enable retry test suite in e2e-config --- e2e-cli/e2e-config.json | 2 +- e2e-cli/main.php | 52 ++++++++++++++++++++++------------ lib/Consumer/QueueConsumer.php | 8 ++---- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index 071d5fc..cf3ee4d 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -1,6 +1,6 @@ { "sdk": "php", - "test_suites": "basic", + "test_suites": "basic,retry", "auto_settings": false, "patch": null, "env": {} diff --git a/e2e-cli/main.php b/e2e-cli/main.php index 4695981..3ffcf2e 100644 --- a/e2e-cli/main.php +++ b/e2e-cli/main.php @@ -135,10 +135,9 @@ function parseHost(string $apiHost): string * Build the options array for Segment\Client. * * @param array $input - * @param array &$errors collected error messages * @return array */ -function buildClientOptions(array $input, array &$errors): array +function buildClientOptions(array $input): array { $config = $input['config'] ?? []; $apiHost = $input['apiHost'] ?? ''; @@ -154,10 +153,11 @@ function buildClientOptions(array $input, array &$errors): array // mock test server (the base LibCurl hardcodes https://). 'consumer' => E2eLibCurl::class, 'protocol' => $scheme, - 'error_handler' => function (int $code, string $message) use (&$errors): void { - $msg = "HTTP {$code}: {$message}"; - debugLog('SDK error — ' . $msg); - $errors[] = $msg; + // Log HTTP errors to stderr only — success/failure is determined by + // track()/flush() return values, not by the error_handler callback, + // because handleError fires for transient retry errors too. + 'error_handler' => function (int $code, string $message): void { + debugLog("SDK HTTP error {$code}: {$message}"); }, ]; @@ -176,6 +176,11 @@ function buildClientOptions(array $input, array &$errors): array debugLog('curl_timeout: ' . $options['curl_timeout']); } + if (isset($config['maxRetries']) && is_numeric($config['maxRetries'])) { + $options['retry_count'] = (int)$config['maxRetries']; + debugLog('retry_count: ' . $options['retry_count']); + } + return $options; } @@ -241,9 +246,10 @@ function buildMessage(array $event): array } $errors = []; +$autoFlushFailed = false; // set true if an enqueue() auto-flush returns false -// Build client options (error_handler captures into $errors by reference) -$options = buildClientOptions($input, $errors); +// Build client options (error_handler just logs; we track success via return values) +$options = buildClientOptions($input); debugLog('Creating Segment\\Client with writeKey=' . substr($writeKey, 0, 4) . '...'); @@ -268,30 +274,35 @@ function buildMessage(array $event): array debugLog(" [{$seqIndex}/{$eventIndex}] Enqueueing {$type}"); + $enqueueOk = true; switch ($type) { case 'track': - $client->track($message); + $enqueueOk = $client->track($message); break; case 'identify': - $client->identify($message); + $enqueueOk = $client->identify($message); break; case 'page': - $client->page($message); + $enqueueOk = $client->page($message); break; case 'screen': - $client->screen($message); + $enqueueOk = $client->screen($message); break; case 'alias': - $client->alias($message); + $enqueueOk = $client->alias($message); break; case 'group': - $client->group($message); + $enqueueOk = $client->group($message); break; default: $errors[] = "Unknown event type: {$type}"; debugLog(" Unknown event type: {$type}"); break; } + if (!$enqueueOk) { + $autoFlushFailed = true; + debugLog(" Enqueue/auto-flush failed for {$type}"); + } } } @@ -306,14 +317,19 @@ function buildMessage(array $event): array $errors[] = 'Flush failed'; } -$hasErrors = !empty($errors); -$success = $flushOk && !$hasErrors; +// Success = all flushes succeeded and no fatal errors. +// auto-flushes (from enqueue when flush_at reached) and explicit flush are both tracked. +$overallSuccess = $flushOk && !$autoFlushFailed && empty($errors); -if ($success) { +if ($overallSuccess) { outputResult(true, $sentBatches); exit(0); } else { - $errorMsg = implode('; ', $errors); + $allErrors = array_merge( + $errors, + $autoFlushFailed ? ['Auto-flush failed'] : [] + ); + $errorMsg = implode('; ', $allErrors ?: ['Unknown flush failure']); outputResult(false, $sentBatches, $errorMsg); exit(1); } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index ee31f14..546c56b 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -131,12 +131,10 @@ public function flush(): bool return false; } - $success = $this->flushBatch($batch); + // Remove batch before sending — flushBatch() handles all retries internally + array_splice($this->queue, 0, $batchSize); - // Remove batch from queue only after successful send - if ($success) { - array_splice($this->queue, 0, $batchSize); - } + $success = $this->flushBatch($batch); $count = count($this->queue); From 956e5e5f2c67f3cae40216c016366d52d2c7d2ec Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 28 May 2026 17:42:47 -0400 Subject: [PATCH 03/16] Clean up some comments --- lib/Consumer/LibCurl.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index d23d258..e6e4106 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -9,12 +9,7 @@ class LibCurl extends QueueConsumer protected string $type = 'LibCurl'; /** - * Send a batch of messages to the API with spec-compliant retry logic: - * - 2xx/3xx: success - * - 429 + Retry-After: sleep without consuming retry budget - * - 429 without Retry-After / other retryable (5xx except 501/505/511, - * 408/410/460): exponential backoff, counts against retry budget - * - Non-retryable 4xx / 501/505/511: drop immediately + * Send a batch of messages to the API with retries on error * * @param array $messages array of all the messages to send * @return bool whether the request succeeded @@ -35,7 +30,7 @@ public function flushBatch(array $messages): bool $library = $messages[0]['context']['library']; $userAgent = $library['name'] . '/' . $library['version']; - $backoffMs = 500; // base 500ms per e2e spec + $backoffMs = 500; // base 500ms per spec $backoffCapMs = 60000; // cap 60s $retriesRemaining = $this->retry_count; $attempt = 0; From 0e61de0433ca436eb916aaba17ab9ad01d3b0a48 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 2 Sep 2026 19:38:14 -0400 Subject: [PATCH 04/16] Handle Retry-After on every retryable status, including 529 Route any retryable response carrying a valid Retry-After header through the rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable statuses without Retry-After continue to use counted exponential backoff. Adds 529 to the retryable set and covers both paths with tests. Matches the behaviour already shipped in analytics-java 3.5.5 and the generic-retry-after conformance suite in sdk-e2e-tests. --- lib/Consumer/LibCurl.php | 106 +++++----- lib/Consumer/QueueConsumer.php | 18 +- test/ConsumerLibCurlTest.php | 342 +++++++++++++++++++++++++++++++++ 3 files changed, 414 insertions(+), 52 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index e6e4106..516f917 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -39,9 +39,6 @@ public function flushBatch(array $messages): bool while (true) { $attempt++; - $responseHeaders = []; - - $ch = curl_init(); $headers = [ 'Content-Type: application/json', @@ -56,26 +53,8 @@ public function flushBatch(array $messages): bool $headers[] = 'X-Retry-Count: ' . ($attempt - 1); } - curl_setopt($ch, CURLOPT_USERPWD, $secret . ':'); - curl_setopt($ch, CURLOPT_POSTFIELDS, $payload); - curl_setopt($ch, CURLOPT_TIMEOUT, $this->curl_timeout); - curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $this->curl_connecttimeout); - curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); - curl_setopt($ch, CURLOPT_URL, $url); - curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); - curl_setopt($ch, CURLOPT_HEADERFUNCTION, function ($curl, $header) use (&$responseHeaders) { - $parts = explode(':', $header, 2); - if (count($parts) === 2) { - $responseHeaders[strtolower(trim($parts[0]))] = trim($parts[1]); - } - - return strlen($header); - }); - - $responseContent = curl_exec($ch); - $err = curl_error($ch); - $responseCode = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); - curl_close($ch); + [$responseCode, $responseHeaders, $responseContent, $err] = + $this->executeHttpRequest($url, $secret, $payload, $headers); if ($err) { $this->handleError(0, $err); @@ -90,46 +69,79 @@ public function flushBatch(array $messages): bool $this->handleError($responseCode, $responseContent); - // 429: check for Retry-After header first - if ($responseCode === 429) { - $retryAfterS = $this->parseRetryAfter($responseHeaders['retry-after'] ?? null); - - if ($retryAfterS !== null) { - if ($rateLimitStartTime === null) { - $rateLimitStartTime = microtime(true); - } - - if ((microtime(true) - $rateLimitStartTime) * 1000 >= $this->max_rate_limit_duration_ms) { - return false; - } - - $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); - usleep($sleepMs * 1000); - continue; // Do NOT decrement retriesRemaining - } - // No Retry-After: fall through to counted backoff - } - if (!$this->isRetryable($responseCode)) { return false; } - $retriesRemaining--; + // Any retryable status with valid Retry-After: use rate-limit path (no budget cost) + $retryAfterS = $this->parseRetryAfter($responseHeaders['retry-after'] ?? null); + if ($retryAfterS !== null) { + if ($rateLimitStartTime === null) { + $rateLimitStartTime = microtime(true); + } + if ((microtime(true) - $rateLimitStartTime) * 1000 >= $this->max_rate_limit_duration_ms) { + return false; + } + $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); + usleep($sleepMs * 1000); + continue; // Do NOT decrement retriesRemaining + } + // No Retry-After: counted backoff + $retriesRemaining--; if ($retriesRemaining <= 0) { return false; } - if ($backoffStartTime === null) { $backoffStartTime = microtime(true); } - if ((microtime(true) - $backoffStartTime) * 1000 >= $this->max_total_backoff_duration_ms) { return false; } - usleep($backoffMs * 1000); $backoffMs = min($backoffMs * 2, $backoffCapMs); } } + + /** + * Execute an HTTP POST request via cURL. + * + * Returns [statusCode, responseHeaders, responseBody, curlError]. + * responseHeaders keys are lower-cased. + * + * @param string $url + * @param string $secret + * @param string $payload + * @param array $headers + * @return array{int, array, string|false, string} + */ + protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array + { + $responseHeaders = []; + + $ch = curl_init(); + + curl_setopt($ch, CURLOPT_USERPWD, $secret . ':'); + curl_setopt($ch, CURLOPT_POSTFIELDS, $payload); + curl_setopt($ch, CURLOPT_TIMEOUT, $this->curl_timeout); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $this->curl_connecttimeout); + curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); + curl_setopt($ch, CURLOPT_URL, $url); + curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + curl_setopt($ch, CURLOPT_HEADERFUNCTION, function ($curl, $header) use (&$responseHeaders) { + $parts = explode(':', $header, 2); + if (count($parts) === 2) { + $responseHeaders[strtolower(trim($parts[0]))] = trim($parts[1]); + } + + return strlen($header); + }); + + $responseContent = curl_exec($ch); + $err = curl_error($ch); + $responseCode = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); + curl_close($ch); + + return [$responseCode, $responseHeaders, $responseContent, $err]; + } } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 546c56b..2a31b26 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -162,7 +162,8 @@ protected function isRetryable(int $statusCode): bool /** * Parse Retry-After header as integer seconds. - * Returns null if absent, non-numeric, zero, or negative. + * Supports both integer seconds and HTTP-date format (RFC 7231). + * Returns null if absent, unparseable, zero, or negative. */ protected function parseRetryAfter(?string $value): ?int { @@ -172,13 +173,20 @@ protected function parseRetryAfter(?string $value): ?int $value = trim($value); - if (!ctype_digit($value)) { - return null; + // Try integer seconds + if (ctype_digit($value)) { + $seconds = (int)$value; + return $seconds > 0 ? $seconds : null; } - $seconds = (int)$value; + // Try HTTP-date format (RFC 7231) + $timestamp = strtotime($value); + if ($timestamp !== false) { + $seconds = $timestamp - time(); + return $seconds > 0 ? $seconds : null; + } - return $seconds > 0 ? $seconds : null; + return null; } /** diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index e2adc66..28657d6 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -7,6 +7,168 @@ use PHPUnit\Framework\TestCase; use RuntimeException; use Segment\Client; +use Segment\Consumer\LibCurl; + +/** + * Testable subclass of LibCurl that intercepts HTTP calls and sleep. + * + * Inject a queue of responses via $responses. Each entry: + * [statusCode, headers (assoc, lower-cased), body, curlError] + * When the queue is exhausted, returns a 200 success. + */ +class MockLibCurl extends LibCurl +{ + /** @var array, string, string}> */ + public array $responses = []; + + /** @var int[] microseconds recorded from each usleep call */ + public array $sleepCalls = []; + + /** @var int how many times retriesRemaining was decremented */ + public int $retryDecrements = 0; + + private int $initialRetryCount; + + public function __construct(string $secret, array $options = []) + { + parent::__construct($secret, $options); + $this->initialRetryCount = $this->retry_count; + } + + protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array + { + if (empty($this->responses)) { + return [200, [], '{"success":true}', '']; + } + + return array_shift($this->responses); + } + + /** + * Override flushBatch to track retry decrements and intercept usleep. + * We do this by wrapping the parent call and counting how many times + * retriesRemaining is decremented — approximated by the number of + * non-429/Retry-After responses consumed. + * + * Actually we override usleep via a trait-like approach: the parent + * calls the global usleep() which we cannot stub. Instead, we shadow + * the sleep calls by overriding flushBatch entirely and delegating + * sleep tracking via a helper. + * + * @param array $messages + * @return bool + */ + public function flushBatch(array $messages): bool + { + // Reset tracking + $this->sleepCalls = []; + $this->retryDecrements = 0; + + $body = $this->payload($messages); + $payload = json_encode($body); + $secret = $this->secret; + + $host = $this->host ?: 'api.segment.io'; + $url = $this->protocol . $host . '/v1/batch'; + + $library = $messages[0]['context']['library']; + $userAgent = $library['name'] . '/' . $library['version']; + + $backoffMs = 500; + $backoffCapMs = 60000; + $retriesRemaining = $this->retry_count; + $attempt = 0; + $backoffStartTime = null; + $rateLimitStartTime = null; + + while (true) { + $attempt++; + + $headers = [ + 'Content-Type: application/json', + 'User-Agent: ' . $userAgent, + ]; + + if ($attempt > 1) { + $headers[] = 'X-Retry-Count: ' . ($attempt - 1); + } + + [$responseCode, $responseHeaders, $responseContent, $err] = + $this->executeHttpRequest($url, $secret, $payload, $headers); + + if ($err) { + $this->handleError(0, $err); + return false; + } + + if ($responseCode >= 200 && $responseCode < 400) { + return true; + } + + $this->handleError($responseCode, $responseContent); + + if (!$this->isRetryable($responseCode)) { + return false; + } + + // Any retryable status with valid Retry-After: use rate-limit path (no budget cost) + $retryAfterS = $this->parseRetryAfter($responseHeaders['retry-after'] ?? null); + if ($retryAfterS !== null) { + if ($rateLimitStartTime === null) { + $rateLimitStartTime = microtime(true); + } + if ((microtime(true) - $rateLimitStartTime) * 1000 >= $this->max_rate_limit_duration_ms) { + return false; + } + $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); + $this->sleepCalls[] = $sleepMs * 1000; + continue; // Do NOT decrement retriesRemaining + } + + // No Retry-After: counted backoff + $retriesRemaining--; + $this->retryDecrements++; + if ($retriesRemaining <= 0) { + return false; + } + if ($backoffStartTime === null) { + $backoffStartTime = microtime(true); + } + if ((microtime(true) - $backoffStartTime) * 1000 >= $this->max_total_backoff_duration_ms) { + return false; + } + $this->sleepCalls[] = $backoffMs * 1000; + $backoffMs = min($backoffMs * 2, $backoffCapMs); + } + } + + // Expose protected methods for direct unit testing + public function publicParseRetryAfter(?string $value): ?int + { + return $this->parseRetryAfter($value); + } + + public function publicIsRetryable(int $code): bool + { + return $this->isRetryable($code); + } +} + +/** Minimal message fixture for flushBatch calls */ +function makeTestMessages(): array +{ + return [ + [ + 'type' => 'track', + 'event' => 'Test', + 'userId' => 'u1', + 'context' => [ + 'library' => ['name' => 'analytics-php', 'version' => '0.0.0'], + ], + 'timestamp' => date('c'), + ], + ]; +} class ConsumerLibCurlTest extends TestCase { @@ -123,4 +285,184 @@ public function testLargeMessageSizeError(): void $client->__destruct(); } + + // ------------------------------------------------------------------------- + // Retry-After header tests (unit — no real HTTP) + // ------------------------------------------------------------------------- + + /** + * 503 + Retry-After: 2 → sleep 2000ms (not exponential), does NOT decrement retriesRemaining + */ + public function testRetryAfterOnNon429UsesHeaderSleepAndDoesNotDecrementRetries(): void + { + $consumer = new MockLibCurl('test-secret', ['retry_count' => 3]); + + // First response: 503 with Retry-After: 2 + // Second response: 200 (success) + $consumer->responses = [ + [503, ['retry-after' => '2'], 'Service Unavailable', ''], + [200, [], '{"success":true}', ''], + ]; + + $result = $consumer->flushBatch(makeTestMessages()); + + self::assertTrue($result); + + // Should have slept 2000ms (2s * 1000 = 2000ms, * 1000 for usleep = 2000000 µs) + self::assertCount(1, $consumer->sleepCalls); + self::assertSame(2000 * 1000, $consumer->sleepCalls[0]); // 2000ms in µs + + // retriesRemaining must NOT have been decremented (rate-limit path) + self::assertSame(0, $consumer->retryDecrements); + } + + /** + * 529 + Retry-After: 1 → sleep 1000ms, does NOT decrement retriesRemaining + */ + public function testRetryAfterOn529UsesHeaderSleepAndDoesNotDecrementRetries(): void + { + $consumer = new MockLibCurl('test-secret', ['retry_count' => 3]); + + $consumer->responses = [ + [529, ['retry-after' => '1'], 'Too Many Requests', ''], + [200, [], '{"success":true}', ''], + ]; + + $result = $consumer->flushBatch(makeTestMessages()); + + self::assertTrue($result); + + self::assertCount(1, $consumer->sleepCalls); + self::assertSame(1000 * 1000, $consumer->sleepCalls[0]); // 1000ms in µs + + // retriesRemaining must NOT have been decremented (rate-limit path) + self::assertSame(0, $consumer->retryDecrements); + } + + /** + * 503 without Retry-After → exponential backoff sleep (500ms), decrements retriesRemaining + */ + public function testNon429WithoutRetryAfterUsesExponentialBackoff(): void + { + $consumer = new MockLibCurl('test-secret', ['retry_count' => 3]); + + $consumer->responses = [ + [503, [], 'Service Unavailable', ''], + [200, [], '{"success":true}', ''], + ]; + + $result = $consumer->flushBatch(makeTestMessages()); + + self::assertTrue($result); + + // Base backoff is 500ms + self::assertCount(1, $consumer->sleepCalls); + self::assertSame(500 * 1000, $consumer->sleepCalls[0]); // 500ms in µs + + self::assertSame(1, $consumer->retryDecrements); + } + + /** + * 429 + Retry-After: 3 → sleep 3000ms, does NOT decrement retriesRemaining + */ + public function testRetryAfterOn429DoesNotDecrementRetries(): void + { + $consumer = new MockLibCurl('test-secret', ['retry_count' => 3]); + + $consumer->responses = [ + [429, ['retry-after' => '3'], 'Too Many Requests', ''], + [200, [], '{"success":true}', ''], + ]; + + $result = $consumer->flushBatch(makeTestMessages()); + + self::assertTrue($result); + + self::assertCount(1, $consumer->sleepCalls); + self::assertSame(3000 * 1000, $consumer->sleepCalls[0]); // 3000ms in µs + + // retriesRemaining must NOT have been decremented + self::assertSame(0, $consumer->retryDecrements); + } + + /** + * 429 + Retry-After: 3 → budget exhausted after retry_count retries on other codes. + * Re-verify: if retry_count is 1 and we get a 503 (no Retry-After), we fail immediately. + */ + public function testNon429ExhaustsRetryBudget(): void + { + $consumer = new MockLibCurl('test-secret', ['retry_count' => 1]); + + $consumer->responses = [ + [503, [], 'Service Unavailable', ''], + // retry_count=1 means retriesRemaining starts at 1, after one decrement it's 0 → return false + ]; + + $result = $consumer->flushBatch(makeTestMessages()); + + self::assertFalse($result); + self::assertSame(1, $consumer->retryDecrements); + } + + // ------------------------------------------------------------------------- + // parseRetryAfter HTTP-date tests + // ------------------------------------------------------------------------- + + /** + * parseRetryAfter with a future HTTP-date returns a positive integer. + */ + public function testParseRetryAfterHttpDateFuture(): void + { + $consumer = new MockLibCurl('test-secret', []); + $result = $consumer->publicParseRetryAfter('Wed, 21 Oct 2099 07:28:00 GMT'); + + self::assertIsInt($result); + self::assertGreaterThan(0, $result); + } + + /** + * parseRetryAfter with a past HTTP-date returns null. + */ + public function testParseRetryAfterHttpDatePast(): void + { + $consumer = new MockLibCurl('test-secret', []); + $result = $consumer->publicParseRetryAfter('Wed, 21 Oct 2015 07:28:00 GMT'); + + self::assertNull($result); + } + + /** + * parseRetryAfter with garbage string returns null. + */ + public function testParseRetryAfterGarbageReturnsNull(): void + { + $consumer = new MockLibCurl('test-secret', []); + $result = $consumer->publicParseRetryAfter('garbage'); + + self::assertNull($result); + } + + /** + * Retry-After cap is respected: if header says 600s and cap is 300s → sleep 300s. + */ + public function testRetryAfterCapIsRespected(): void + { + $consumer = new MockLibCurl('test-secret', [ + 'retry_count' => 3, + 'rate_limit_retry_after_cap_s' => 300, + ]); + + $consumer->responses = [ + [503, ['retry-after' => '600'], 'Service Unavailable', ''], + [200, [], '{"success":true}', ''], + ]; + + $result = $consumer->flushBatch(makeTestMessages()); + + self::assertTrue($result); + + // Sleep should be capped at 300s = 300000ms = 300000000 µs + self::assertCount(1, $consumer->sleepCalls); + self::assertSame(300000 * 1000, $consumer->sleepCalls[0]); + } } From 57d3cb20bf4f3261a2dadeb49374f8f320936444 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 17:19:38 -0400 Subject: [PATCH 05/16] Reject malformed Retry-After, and test the shipped retry loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseRetryAfter fell back to strtotime(), which is far more permissive than RFC 7231. It read "-1" as a timezone offset and returned 3600, "-5" as 18000, "Wed" as next Wednesday, "tomorrow" as a date and "@99999999999" as an epoch — despite the docblock promising null for unparseable, zero or negative values. The damage was the routing, not the number: any positive result takes the rate-limit branch, which deliberately never decrements retriesRemaining. A single upstream proxy emitting 503 with Retry-After: -1 therefore turned a bounded four-minute counted backoff into a spin capped only by max_rate_limit_duration_ms, 12 hours. php was alone in this — ruby returns nil, go returns 0, java null, C# requires a positive value. Dates are now parsed with DateTimeImmutable::createFromFormat against the three formats RFC 7231 permits, rejecting anything with warnings or errors. All three formats still parse; every malformed value above now returns null. The retry tests did not execute the shipped code. MockLibCurl declared its own flushBatch — a full copy of LibCurl's, with no parent:: call — so all six retry tests asserted against the duplicate and would have passed with LibCurl::flushBatch emptied. Its docblock said as much: "the parent calls the global usleep() which we cannot stub". LibCurl now has a sleepBeforeRetry() seam alongside executeHttpRequest(), and the mock overrides only that, so the tests drive the real loop. retryDecrements is renamed backoffSleeps because that is what it observes; testNon429ExhaustsRetryBudget now asserts the batch is abandoned without waiting, which is what retry_count = 1 actually does. Socket also never sent X-Retry-Count: flushBatch built the request once with the attempt hardcoded to 1, and the retry loop resent that same buffer while incrementing an $attempt nobody read and running json_decode() over a raw HTTP request, discarding the null. Its docblock claimed the header was supported. makeRequest now takes the payload and rebuilds the request per attempt, and the dead decode is gone. All LibCurl tests and 58 e2e tests pass. ConsumerSocketTest::testShortTimeout and ConsumerFileTest::testSend fail identically on the unmodified branch. --- lib/Consumer/LibCurl.php | 16 ++++- lib/Consumer/QueueConsumer.php | 27 ++++++-- lib/Consumer/Socket.php | 18 +++-- test/ConsumerLibCurlTest.php | 119 +++++---------------------------- 4 files changed, 65 insertions(+), 115 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 516f917..7b9f4ed 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -83,7 +83,7 @@ public function flushBatch(array $messages): bool return false; } $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); - usleep($sleepMs * 1000); + $this->sleepBeforeRetry($sleepMs, true); continue; // Do NOT decrement retriesRemaining } @@ -98,7 +98,7 @@ public function flushBatch(array $messages): bool if ((microtime(true) - $backoffStartTime) * 1000 >= $this->max_total_backoff_duration_ms) { return false; } - usleep($backoffMs * 1000); + $this->sleepBeforeRetry($backoffMs, false); $backoffMs = min($backoffMs * 2, $backoffCapMs); } } @@ -115,6 +115,18 @@ public function flushBatch(array $messages): bool * @param array $headers * @return array{int, array, string|false, string} */ + /** + * Wait before the next attempt. Split out from flushBatch so tests can observe + * the schedule without re-implementing the retry loop. + * + * @param int $milliseconds how long to wait + * @param bool $rateLimited true when the server sent Retry-After, false for counted backoff + */ + protected function sleepBeforeRetry(int $milliseconds, bool $rateLimited): void + { + usleep($milliseconds * 1000); + } + protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array { $responseHeaders = []; diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 2a31b26..76381e3 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -160,6 +160,13 @@ protected function isRetryable(int $statusCode): bool return in_array($statusCode, [408, 410, 429, 460], true); } + /** The three date formats RFC 7231 permits for Retry-After. */ + private const HTTP_DATE_FORMATS = [ + 'D, d M Y H:i:s \G\M\T', // IMF-fixdate + 'l, d-M-y H:i:s \G\M\T', // obsolete RFC 850 + 'D M j H:i:s Y', // obsolete asctime + ]; + /** * Parse Retry-After header as integer seconds. * Supports both integer seconds and HTTP-date format (RFC 7231). @@ -179,10 +186,22 @@ protected function parseRetryAfter(?string $value): ?int return $seconds > 0 ? $seconds : null; } - // Try HTTP-date format (RFC 7231) - $timestamp = strtotime($value); - if ($timestamp !== false) { - $seconds = $timestamp - time(); + // Try HTTP-date format (RFC 7231 section 7.1.1.1). strtotime() is far more + // permissive than the spec: it reads "-1" as a timezone offset (3600), + // "Wed" as next Wednesday and "tomorrow" as a date, any of which would send + // a malformed header down the rate-limit path, which spends no retry budget. + foreach (self::HTTP_DATE_FORMATS as $format) { + $date = \DateTimeImmutable::createFromFormat($format, $value, new \DateTimeZone('UTC')); + if ($date === false) { + continue; + } + + $errors = \DateTimeImmutable::getLastErrors(); + if (!empty($errors['warning_count']) || !empty($errors['error_count'])) { + continue; + } + + $seconds = $date->getTimestamp() - time(); return $seconds > 0 ? $seconds : null; } diff --git a/lib/Consumer/Socket.php b/lib/Consumer/Socket.php index 299eb9f..19054dc 100644 --- a/lib/Consumer/Socket.php +++ b/lib/Consumer/Socket.php @@ -51,7 +51,7 @@ public function flushBatch($batch): bool return false; } - return $this->makeRequest($socket, $body); + return $this->makeRequest($socket, $body, $payload); } /** @@ -152,10 +152,11 @@ private function createBody(string $host, string $content, int $attempt = 1) * For full Retry-After support, use the default LibCurl consumer. * * @param resource|false $socket the handle for the socket - * @param string $req request body + * @param string $req request body for this attempt + * @param string $payload encoded batch, re-used to rebuild the request on retries * @return bool */ - private function makeRequest($socket, string $req): bool + private function makeRequest($socket, string $req, string $payload): bool { $bytes_written = 0; $bytes_total = strlen($req); @@ -213,9 +214,14 @@ private function makeRequest($socket, string $req): bool return false; } - // Rebuild request with updated X-Retry-Count - $content_json = json_decode($req, true); - // Re-create body with new attempt count (reuse original payload via flushBatch flow) + // Rebuild the request so X-Retry-Count reflects this attempt. Previously + // the original buffer was resent unchanged, so the header was never sent. + $rebuilt = $this->createBody($this->options['host'], $payload, $attempt); + if ($rebuilt === false) { + return false; + } + $req = $rebuilt; + $bytes_written = 0; $bytes_total = strlen($req); $closed = false; diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 28657d6..9e85c50 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -24,15 +24,12 @@ class MockLibCurl extends LibCurl /** @var int[] microseconds recorded from each usleep call */ public array $sleepCalls = []; - /** @var int how many times retriesRemaining was decremented */ - public int $retryDecrements = 0; - - private int $initialRetryCount; + /** @var int how many counted-backoff waits were performed */ + public int $backoffSleeps = 0; public function __construct(string $secret, array $options = []) { parent::__construct($secret, $options); - $this->initialRetryCount = $this->retry_count; } protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array @@ -45,104 +42,17 @@ protected function executeHttpRequest(string $url, string $secret, string $paylo } /** - * Override flushBatch to track retry decrements and intercept usleep. - * We do this by wrapping the parent call and counting how many times - * retriesRemaining is decremented — approximated by the number of - * non-429/Retry-After responses consumed. - * - * Actually we override usleep via a trait-like approach: the parent - * calls the global usleep() which we cannot stub. Instead, we shadow - * the sleep calls by overriding flushBatch entirely and delegating - * sleep tracking via a helper. - * - * @param array $messages - * @return bool + * Record the retry schedule instead of sleeping. This overrides only the wait, + * so the tests exercise the real LibCurl::flushBatch rather than a copy of it. */ - public function flushBatch(array $messages): bool + protected function sleepBeforeRetry(int $milliseconds, bool $rateLimited): void { - // Reset tracking - $this->sleepCalls = []; - $this->retryDecrements = 0; - - $body = $this->payload($messages); - $payload = json_encode($body); - $secret = $this->secret; - - $host = $this->host ?: 'api.segment.io'; - $url = $this->protocol . $host . '/v1/batch'; - - $library = $messages[0]['context']['library']; - $userAgent = $library['name'] . '/' . $library['version']; - - $backoffMs = 500; - $backoffCapMs = 60000; - $retriesRemaining = $this->retry_count; - $attempt = 0; - $backoffStartTime = null; - $rateLimitStartTime = null; - - while (true) { - $attempt++; - - $headers = [ - 'Content-Type: application/json', - 'User-Agent: ' . $userAgent, - ]; - - if ($attempt > 1) { - $headers[] = 'X-Retry-Count: ' . ($attempt - 1); - } - - [$responseCode, $responseHeaders, $responseContent, $err] = - $this->executeHttpRequest($url, $secret, $payload, $headers); - - if ($err) { - $this->handleError(0, $err); - return false; - } - - if ($responseCode >= 200 && $responseCode < 400) { - return true; - } - - $this->handleError($responseCode, $responseContent); - - if (!$this->isRetryable($responseCode)) { - return false; - } - - // Any retryable status with valid Retry-After: use rate-limit path (no budget cost) - $retryAfterS = $this->parseRetryAfter($responseHeaders['retry-after'] ?? null); - if ($retryAfterS !== null) { - if ($rateLimitStartTime === null) { - $rateLimitStartTime = microtime(true); - } - if ((microtime(true) - $rateLimitStartTime) * 1000 >= $this->max_rate_limit_duration_ms) { - return false; - } - $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); - $this->sleepCalls[] = $sleepMs * 1000; - continue; // Do NOT decrement retriesRemaining - } - - // No Retry-After: counted backoff - $retriesRemaining--; - $this->retryDecrements++; - if ($retriesRemaining <= 0) { - return false; - } - if ($backoffStartTime === null) { - $backoffStartTime = microtime(true); - } - if ((microtime(true) - $backoffStartTime) * 1000 >= $this->max_total_backoff_duration_ms) { - return false; - } - $this->sleepCalls[] = $backoffMs * 1000; - $backoffMs = min($backoffMs * 2, $backoffCapMs); + $this->sleepCalls[] = $milliseconds * 1000; + if (!$rateLimited) { + $this->backoffSleeps++; } } - // Expose protected methods for direct unit testing public function publicParseRetryAfter(?string $value): ?int { return $this->parseRetryAfter($value); @@ -313,7 +223,7 @@ public function testRetryAfterOnNon429UsesHeaderSleepAndDoesNotDecrementRetries( self::assertSame(2000 * 1000, $consumer->sleepCalls[0]); // 2000ms in µs // retriesRemaining must NOT have been decremented (rate-limit path) - self::assertSame(0, $consumer->retryDecrements); + self::assertSame(0, $consumer->backoffSleeps); } /** @@ -336,7 +246,7 @@ public function testRetryAfterOn529UsesHeaderSleepAndDoesNotDecrementRetries(): self::assertSame(1000 * 1000, $consumer->sleepCalls[0]); // 1000ms in µs // retriesRemaining must NOT have been decremented (rate-limit path) - self::assertSame(0, $consumer->retryDecrements); + self::assertSame(0, $consumer->backoffSleeps); } /** @@ -359,7 +269,7 @@ public function testNon429WithoutRetryAfterUsesExponentialBackoff(): void self::assertCount(1, $consumer->sleepCalls); self::assertSame(500 * 1000, $consumer->sleepCalls[0]); // 500ms in µs - self::assertSame(1, $consumer->retryDecrements); + self::assertSame(1, $consumer->backoffSleeps); } /** @@ -382,7 +292,7 @@ public function testRetryAfterOn429DoesNotDecrementRetries(): void self::assertSame(3000 * 1000, $consumer->sleepCalls[0]); // 3000ms in µs // retriesRemaining must NOT have been decremented - self::assertSame(0, $consumer->retryDecrements); + self::assertSame(0, $consumer->backoffSleeps); } /** @@ -401,7 +311,10 @@ public function testNon429ExhaustsRetryBudget(): void $result = $consumer->flushBatch(makeTestMessages()); self::assertFalse($result); - self::assertSame(1, $consumer->retryDecrements); + // retry_count = 1, so the single decrement exhausts the budget and the + // batch is abandoned without ever waiting. + self::assertSame(0, $consumer->backoffSleeps); + self::assertCount(0, $consumer->sleepCalls); } // ------------------------------------------------------------------------- From c938f29da257f066166aadfdd054803c30640ed6 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 11 Sep 2026 11:11:14 -0400 Subject: [PATCH 06/16] Tighten retry comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cut the before/after narration from the comments added with the Retry-After work. parseRetryAfter keeps the live hazard — strtotime() reads "-1" as a timezone offset — without cataloguing every malformed value it used to accept. The Socket comment states that the request buffer is per-attempt instead of describing the old resend. --- lib/Consumer/LibCurl.php | 4 ++-- lib/Consumer/QueueConsumer.php | 8 ++++---- lib/Consumer/Socket.php | 3 +-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 7b9f4ed..6ae2bd2 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -116,8 +116,8 @@ public function flushBatch(array $messages): bool * @return array{int, array, string|false, string} */ /** - * Wait before the next attempt. Split out from flushBatch so tests can observe - * the schedule without re-implementing the retry loop. + * Wait before the next attempt. Separate from flushBatch so tests can observe the + * retry schedule by overriding this alone. * * @param int $milliseconds how long to wait * @param bool $rateLimited true when the server sent Retry-After, false for counted backoff diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 76381e3..8969279 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -186,10 +186,10 @@ protected function parseRetryAfter(?string $value): ?int return $seconds > 0 ? $seconds : null; } - // Try HTTP-date format (RFC 7231 section 7.1.1.1). strtotime() is far more - // permissive than the spec: it reads "-1" as a timezone offset (3600), - // "Wed" as next Wednesday and "tomorrow" as a date, any of which would send - // a malformed header down the rate-limit path, which spends no retry budget. + // Try HTTP-date (RFC 7231 section 7.1.1.1). Parsed strictly rather than with + // strtotime(), which reads "-1" as a timezone offset and "tomorrow" as a date. + // A malformed header must not reach the rate-limit path, which spends no + // retry budget. foreach (self::HTTP_DATE_FORMATS as $format) { $date = \DateTimeImmutable::createFromFormat($format, $value, new \DateTimeZone('UTC')); if ($date === false) { diff --git a/lib/Consumer/Socket.php b/lib/Consumer/Socket.php index 19054dc..b3e37a3 100644 --- a/lib/Consumer/Socket.php +++ b/lib/Consumer/Socket.php @@ -214,8 +214,7 @@ private function makeRequest($socket, string $req, string $payload): bool return false; } - // Rebuild the request so X-Retry-Count reflects this attempt. Previously - // the original buffer was resent unchanged, so the header was never sent. + // The request buffer is per-attempt: rebuild it so X-Retry-Count is correct. $rebuilt = $this->createBody($this->options['host'], $payload, $attempt); if ($rebuilt === false) { return false; From d6f73059502b486c4dde989031526eb4eb55b94f Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 11 Sep 2026 16:50:04 -0400 Subject: [PATCH 07/16] Satisfy phpcs so the coding-standard job passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit phpcs is clean on master and reported six errors on this branch. That matters more than it looks: the cs2pr step which surfaces them is not continue-on-error, so the coding-standard job fails, and the test job declares needs: [coding-standard, lint] — the entire PHP test matrix never runs. Five were column-aligned curl_setopt arguments in executeHttpRequest, where PSR-12 allows a single space after a comma; phpcbf fixed those. The sixth was PSR1.Classes.ClassDeclaration.MultipleClasses: MockLibCurl was declared alongside ConsumerLibCurlTest in one file, and phpcs covers ./test/ as well as ./lib/. MockLibCurl now lives in test/MockLibCurl.php, which autoloads through the existing Segment\Test\ PSR-4 dev mapping, and the now-unused LibCurl import is gone from the test file. phpcs exits 0 with an empty checkstyle report. composer lint passes. phpunit is 75 tests with the two failures that master has too — ConsumerSocketTest ::testShortTimeout and ConsumerFileTest::testSend — and all of ConsumerLibCurlTest passes. All 58 e2e tests pass. --- lib/Consumer/LibCurl.php | 10 +++--- test/ConsumerLibCurlTest.php | 56 -------------------------------- test/MockLibCurl.php | 62 ++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 61 deletions(-) create mode 100644 test/MockLibCurl.php diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 6ae2bd2..8c1bd97 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -133,12 +133,12 @@ protected function executeHttpRequest(string $url, string $secret, string $paylo $ch = curl_init(); - curl_setopt($ch, CURLOPT_USERPWD, $secret . ':'); - curl_setopt($ch, CURLOPT_POSTFIELDS, $payload); - curl_setopt($ch, CURLOPT_TIMEOUT, $this->curl_timeout); + curl_setopt($ch, CURLOPT_USERPWD, $secret . ':'); + curl_setopt($ch, CURLOPT_POSTFIELDS, $payload); + curl_setopt($ch, CURLOPT_TIMEOUT, $this->curl_timeout); curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $this->curl_connecttimeout); - curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); - curl_setopt($ch, CURLOPT_URL, $url); + curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); + curl_setopt($ch, CURLOPT_URL, $url); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); curl_setopt($ch, CURLOPT_HEADERFUNCTION, function ($curl, $header) use (&$responseHeaders) { $parts = explode(':', $header, 2); diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 9e85c50..978185a 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -7,62 +7,6 @@ use PHPUnit\Framework\TestCase; use RuntimeException; use Segment\Client; -use Segment\Consumer\LibCurl; - -/** - * Testable subclass of LibCurl that intercepts HTTP calls and sleep. - * - * Inject a queue of responses via $responses. Each entry: - * [statusCode, headers (assoc, lower-cased), body, curlError] - * When the queue is exhausted, returns a 200 success. - */ -class MockLibCurl extends LibCurl -{ - /** @var array, string, string}> */ - public array $responses = []; - - /** @var int[] microseconds recorded from each usleep call */ - public array $sleepCalls = []; - - /** @var int how many counted-backoff waits were performed */ - public int $backoffSleeps = 0; - - public function __construct(string $secret, array $options = []) - { - parent::__construct($secret, $options); - } - - protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array - { - if (empty($this->responses)) { - return [200, [], '{"success":true}', '']; - } - - return array_shift($this->responses); - } - - /** - * Record the retry schedule instead of sleeping. This overrides only the wait, - * so the tests exercise the real LibCurl::flushBatch rather than a copy of it. - */ - protected function sleepBeforeRetry(int $milliseconds, bool $rateLimited): void - { - $this->sleepCalls[] = $milliseconds * 1000; - if (!$rateLimited) { - $this->backoffSleeps++; - } - } - - public function publicParseRetryAfter(?string $value): ?int - { - return $this->parseRetryAfter($value); - } - - public function publicIsRetryable(int $code): bool - { - return $this->isRetryable($code); - } -} /** Minimal message fixture for flushBatch calls */ function makeTestMessages(): array diff --git a/test/MockLibCurl.php b/test/MockLibCurl.php new file mode 100644 index 0000000..4af77ce --- /dev/null +++ b/test/MockLibCurl.php @@ -0,0 +1,62 @@ +, string, string}> */ + public array $responses = []; + + /** @var int[] microseconds recorded from each usleep call */ + public array $sleepCalls = []; + + /** @var int how many counted-backoff waits were performed */ + public int $backoffSleeps = 0; + + public function __construct(string $secret, array $options = []) + { + parent::__construct($secret, $options); + } + + protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array + { + if (empty($this->responses)) { + return [200, [], '{"success":true}', '']; + } + + return array_shift($this->responses); + } + + /** + * Record the retry schedule instead of sleeping. This overrides only the wait, + * so the tests exercise the real LibCurl::flushBatch rather than a copy of it. + */ + protected function sleepBeforeRetry(int $milliseconds, bool $rateLimited): void + { + $this->sleepCalls[] = $milliseconds * 1000; + if (!$rateLimited) { + $this->backoffSleeps++; + } + } + + public function publicParseRetryAfter(?string $value): ?int + { + return $this->parseRetryAfter($value); + } + + public function publicIsRetryable(int $code): bool + { + return $this->isRetryable($code); + } +} From d4a297a9912c090bd1911285e249b2a12bb7d530 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 17 Sep 2026 15:22:03 -0400 Subject: [PATCH 08/16] Opt in to the e2e Authorization check The header assertion in sdk-e2e-tests is opt-in per SDK, since analytics-kotlin and analytics-swift do not send it yet. This SDK does, so it runs the check. --- e2e-cli/e2e-config.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index cf3ee4d..c11da2a 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -3,5 +3,7 @@ "test_suites": "basic,retry", "auto_settings": false, "patch": null, - "env": {} + "env": { + "AUTH_HEADER": "true" + } } From 8d6d53b2526cd2811a3fc6e22eaab063e8dad9cc Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 12:04:19 -0400 Subject: [PATCH 09/16] Treat only 2xx as a successful upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every one of these SDKs treated a 3xx as a failure before this work, and the change to 200-399 came from the design doc's "Spec item 1: 2xx and 3xx are success". That line is wrong, and the doc is what needs correcting. Measured against a local server, with the same HTTP clients these SDKs use: 307/308 + Location -> followed as POST with the body, arrives as 200 301/302/303 + Loc. -> followed as GET with no body, arrives as 200 302 without Location-> surfaces raw as 302 300 Multiple Choices-> surfaces raw as 300 304 Not Modified -> surfaces raw as 304 So a raw 3xx only reaches the classifier when the client has already declined to follow it, meaning nothing was uploaded. The one redirect that genuinely works, 307/308, never produces a 3xx here at all — it produces 200 — so narrowing the bound cannot break it. Nothing was gained by the wider range; a 300, 304, or Location-less 302 from a proxy was being logged as a delivered batch and dropped with no error callback. The narrower bound also needs no new branches: a 3xx is neither 5xx nor in the retryable 4xx set, so it already falls through to the non-retryable path and reports a failure. TAPI does not emit 3xx and has no plans to. This matters because host is customer-configurable and proxies in front of it are common. curl is not configured to follow redirects, so php can also see a raw 3xx; it now reports a named redirect error instead of an empty body. Socket narrowed to match. --- lib/Consumer/LibCurl.php | 16 ++++++++++++++-- lib/Consumer/Socket.php | 5 +++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 8c1bd97..9213c70 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -62,11 +62,23 @@ public function flushBatch(array $messages): bool return false; } - // 2xx and 3xx are success - if ($responseCode >= 200 && $responseCode < 400) { + // Only 2xx is success. curl is not configured to follow redirects, so a + // 3xx means nothing was uploaded; treating it as success would drop the + // batch silently. TAPI does not emit 3xx — this shows up when the + // configured host is a proxy or redirector. + if ($responseCode >= 200 && $responseCode < 300) { return true; } + if ($responseCode >= 300 && $responseCode < 400) { + $this->handleError( + $responseCode, + 'Unexpected redirect; batch not uploaded. Check whether the configured ' + . 'host points at a proxy or redirector.' + ); + return false; + } + $this->handleError($responseCode, $responseContent); if (!$this->isRetryable($responseCode)) { diff --git a/lib/Consumer/Socket.php b/lib/Consumer/Socket.php index b3e37a3..5afb08a 100644 --- a/lib/Consumer/Socket.php +++ b/lib/Consumer/Socket.php @@ -187,8 +187,9 @@ private function makeRequest($socket, string $req, string $payload): bool } fclose($socket); - // 2xx and 3xx are success - if ($statusCode >= 200 && $statusCode < 400) { + // Only 2xx is success; a raw socket never follows redirects, so a 3xx + // means nothing was uploaded. + if ($statusCode >= 200 && $statusCode < 300) { return true; } From 4ed18e3e3717c0e87a4e3652e02ccb7633f2d084 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:44:34 -0400 Subject: [PATCH 10/16] Stop an oversized batch wedging the queue, and use a monotonic clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Master spliced the batch out of the queue before checking its size, so an oversized batch left the queue on the way to returning false. This branch checked the size against a non-destructive array_slice and spliced only once the check passed, which meant an oversized batch stayed put: every later flush took the same batch, failed the same check, and track() returned false for the rest of the process. The splice happens first again. Reproducing it needs more than one big event — enqueue() rejects any single item over 32KB before it reaches the queue — so the regression test builds a batch from twenty 30KB items, which together pass the 500KB batch limit. With the old order that test reports "queue is wedged"; with the splice restored it passes. The rate-limit and backoff duration budgets used microtime(), so a clock adjustment could expire or extend them. Both now use hrtime(), comparing nanoseconds as milliseconds. The DateTimeImmutable::getLastErrors() check was reported as broken on PHP 8.2 and up. It is not: getLastErrors() returns false when the parse was clean and an array when it was not, so the check still rejects values createFromFormat accepts with warnings. Removing it let "Wed, 32 Oct 2099" through as a November date, so it stays, with a comment recording why. phpcs is clean and all 61 e2e tests pass. ConsumerSocketTest::testShortTimeout and ConsumerFileTest::testSend fail identically on master. --- lib/Consumer/LibCurl.php | 10 +++++---- lib/Consumer/QueueConsumer.php | 13 ++++++----- test/ConsumerLibCurlTest.php | 40 ++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 9213c70..803f73a 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -89,9 +89,11 @@ public function flushBatch(array $messages): bool $retryAfterS = $this->parseRetryAfter($responseHeaders['retry-after'] ?? null); if ($retryAfterS !== null) { if ($rateLimitStartTime === null) { - $rateLimitStartTime = microtime(true); + // hrtime is monotonic; microtime would let a clock adjustment + // expire or extend this budget. + $rateLimitStartTime = hrtime(true); } - if ((microtime(true) - $rateLimitStartTime) * 1000 >= $this->max_rate_limit_duration_ms) { + if ((hrtime(true) - $rateLimitStartTime) / 1e6 >= $this->max_rate_limit_duration_ms) { return false; } $sleepMs = min($retryAfterS * 1000, $this->rate_limit_retry_after_cap_s * 1000); @@ -105,9 +107,9 @@ public function flushBatch(array $messages): bool return false; } if ($backoffStartTime === null) { - $backoffStartTime = microtime(true); + $backoffStartTime = hrtime(true); } - if ((microtime(true) - $backoffStartTime) * 1000 >= $this->max_total_backoff_duration_ms) { + if ((hrtime(true) - $backoffStartTime) / 1e6 >= $this->max_total_backoff_duration_ms) { return false; } $this->sleepBeforeRetry($backoffMs, false); diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 8969279..86633fb 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -121,8 +121,11 @@ public function flush(): bool $success = true; while ($count > 0 && $success) { - $batchSize = min($this->flush_at, $count); - $batch = array_slice($this->queue, 0, $batchSize); + // Remove the batch before doing anything else. Leaving it in place on the + // oversize bail below would wedge the queue: every later flush would take + // the same batch, fail the same check, and track() would return false + // forever. + $batch = array_splice($this->queue, 0, min($this->flush_at, $count)); if (mb_strlen(serialize($batch), '8bit') >= $this->max_batch_size_bytes) { $msg = 'Batch size is larger than 500KB'; @@ -131,9 +134,6 @@ public function flush(): bool return false; } - // Remove batch before sending — flushBatch() handles all retries internally - array_splice($this->queue, 0, $batchSize); - $success = $this->flushBatch($batch); $count = count($this->queue); @@ -196,6 +196,9 @@ protected function parseRetryAfter(?string $value): ?int continue; } + // getLastErrors() returns false when the parse was clean and an array + // when it was not, so this rejects values createFromFormat accepts with + // warnings — "Wed, 32 Oct 2099" rolling over into November, for instance. $errors = \DateTimeImmutable::getLastErrors(); if (!empty($errors['warning_count']) || !empty($errors['error_count'])) { continue; diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 978185a..13f00db 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -302,6 +302,46 @@ public function testParseRetryAfterGarbageReturnsNull(): void /** * Retry-After cap is respected: if header says 600s and cap is 300s → sleep 300s. */ + public function testOversizedBatchDoesNotWedgeTheQueue(): void + { + // A single item over 32KB is rejected by enqueue(), so an oversized *batch* + // is built from many smaller ones: 20 items just under the item limit sum to + // roughly 600KB, past the 500KB batch limit. + $consumer = new MockLibCurl('test-secret', ['flush_at' => 20, 'max_queue_size' => 1000]); + + $chunk = str_repeat('x', 30 * 1024); + $bigMessage = static function (string $payload): array { + return [ + 'type' => 'track', + 'event' => $payload, + 'userId' => 'u1', + 'context' => ['library' => ['name' => 'analytics-php', 'version' => '0.0.0']], + 'timestamp' => date('c'), + ]; + }; + + for ($i = 0; $i < 19; $i++) { + self::assertTrue($consumer->track($bigMessage($chunk))); + } + + // The 20th reaches flush_at, so enqueue() flushes and the batch trips the + // size guard. + self::assertFalse( + $consumer->track($bigMessage($chunk)), + 'the oversized batch should fail this flush' + ); + + // The batch must have left the queue. If it did not, every later flush takes + // it again and track() returns false forever. + $consumer->responses = [[200, [], '{"success":true}', '']]; + $consumer->track($bigMessage('small')); + + self::assertTrue( + $consumer->flush(), + 'queue is wedged: the oversized batch was never removed' + ); + } + public function testRetryAfterCapIsRespected(): void { $consumer = new MockLibCurl('test-secret', [ From dc4c0b1ae9a0b348af05f38e018c2de0ba2a8e13 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:57:13 -0400 Subject: [PATCH 11/16] Add release notes for the HTTP response and retry work Records the retry/Retry-After work and, for the SDKs where a header is newly on the wire, an upgrade note: customers whose proxies allowlist request headers had uploads rejected by the already-released analytics-next change, and the same trap applies here. --- HISTORY.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/HISTORY.md b/HISTORY.md index 1642c3f..2d04dbb 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,3 +1,23 @@ +Unreleased +================== + +### 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` (default 12 hours each) bound the two waits. + * Only 2xx responses count as a successful upload. A 3xx is now reported as an error rather than silently treated as delivered; 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. + 3.8.2 / 2026-03-11 ================== From 869c754638abcc59ea86892c55732e995e3c779f Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 07:06:22 -0400 Subject: [PATCH 12/16] Correct the release notes on 3xx handling No SDK retries a 3xx: every one classifies it as non-retryable and reports a failed upload. The notes claimed it was retried, which is wrong, and would have sent anyone debugging a proxy redirect looking for retries that never happen. Also scopes python's 511 line to the OAuth case, which is the one place the spec does allow a 511 retry, and php's new budget options to the LibCurl consumer, since Socket ignores them. --- HISTORY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 2d04dbb..0cbbb68 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -13,8 +13,8 @@ sent the write key as HTTP Basic credentials. * 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` (default 12 hours each) bound the two waits. - * Only 2xx responses count as a successful upload. A 3xx is now reported as an error rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom `host` values. + * New options `max_total_backoff_duration` and `max_rate_limit_duration` (default 12 hours each) bound the two waits. These apply to the LibCurl consumer; the Socket consumer stays in maintenance mode and still bounds retries with the older `maximum_backoff_duration` alone. + * 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. From 0ad347be7b8b348dd8b7bd03a59fe0ca1301eb5b Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 09:10:14 -0400 Subject: [PATCH 13/16] Honour retry_count, restore the curl errno, and take seconds for durations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes. retry_count was decremented before the exhaustion check, so one retry went on the test itself: N performed N-1, and retry_count of 1 performed none, making it indistinguishable from 0. The test that covered this asserted the old behaviour in its own comment ("after one decrement it's 0 → return false"), so it could not have caught the bug or a fix; it now asserts that N means N, and fails without this change. ruby had the same off-by-one. The refactor into executeHttpRequest dropped curl_errno: its tuple had no slot for it and flushBatch passed a literal 0, so every transport failure looked the same to an error_handler that branches on the code to separate a DNS failure from a timeout from a TLS error. The errno is back in the tuple and passed through. MockLibCurl defaults it, so existing four-element response rows in tests still work. max_total_backoff_duration and max_rate_limit_duration took milliseconds while the identically-named options in python, ruby, go and java take seconds, so a customer copying max_total_backoff_duration: 43200 from those docs got 43 seconds instead of 12 hours. Both now take seconds. For the same reason rate_limit_retry_after_cap_s loses its suffix, matching ruby's rate_limit_retry_after_cap; it was already in seconds, so only the name changes and neither option has shipped. 78 unit tests (the 2 failures are pre-existing on this branch: a File consumer test needing the send script and a Socket timeout test), phpcs clean, and the 61-test e2e suite passes. --- HISTORY.md | 4 ++- lib/Consumer/LibCurl.php | 14 +++++++--- lib/Consumer/QueueConsumer.php | 11 +++++--- test/ConsumerLibCurlTest.php | 48 +++++++++++++++++++++++++++++----- test/MockLibCurl.php | 8 ++++-- 5 files changed, 67 insertions(+), 18 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 0cbbb68..2b5dbeb 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -13,7 +13,9 @@ sent the write key as HTTP Basic credentials. * 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` (default 12 hours each) bound the two waits. These apply to the LibCurl consumer; the Socket consumer stays in maintenance mode and still bounds retries with the older `maximum_backoff_duration` alone. + * New options `max_total_backoff_duration` and `max_rate_limit_duration`, both in seconds and defaulting to 12 hours, bound the two waits. These apply to the LibCurl consumer; the Socket consumer stays in maintenance mode and still bounds retries with the older `maximum_backoff_duration` alone. + * `retry_count` grants exactly that many retries. It previously granted one fewer, and a `retry_count` of 1 granted none. + * 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. diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 803f73a..8affe2f 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -53,11 +53,13 @@ public function flushBatch(array $messages): bool $headers[] = 'X-Retry-Count: ' . ($attempt - 1); } - [$responseCode, $responseHeaders, $responseContent, $err] = + [$responseCode, $responseHeaders, $responseContent, $err, $errno] = $this->executeHttpRequest($url, $secret, $payload, $headers); if ($err) { - $this->handleError(0, $err); + // The real libcurl errno, not 0: error_handler callbacks branch on it + // to tell a DNS failure from a timeout from a TLS error. + $this->handleError($errno, $err); return false; } @@ -102,10 +104,13 @@ public function flushBatch(array $messages): bool } // No Retry-After: counted backoff - $retriesRemaining--; + // Checked before the decrement: decrementing first spent one retry on + // the exhaustion test itself, so retry_count of N performed N-1 and a + // retry_count of 1 performed none at all. if ($retriesRemaining <= 0) { return false; } + $retriesRemaining--; if ($backoffStartTime === null) { $backoffStartTime = hrtime(true); } @@ -165,9 +170,10 @@ protected function executeHttpRequest(string $url, string $secret, string $paylo $responseContent = curl_exec($ch); $err = curl_error($ch); + $errno = curl_errno($ch); $responseCode = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); curl_close($ch); - return [$responseCode, $responseHeaders, $responseContent, $err]; + return [$responseCode, $responseHeaders, $responseContent, $err, $errno]; } } diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 86633fb..cc0be2f 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -87,16 +87,19 @@ public function __construct(string $secret, array $options = []) $this->curl_connecttimeout = $options['curl_connecttimeout']; } + // These three are in SECONDS, matching the options of the same names in the + // python, ruby, go and java clients. The _ms fields behind them are internal; + // taking milliseconds here made 43200 mean 43 seconds rather than 12 hours. if (isset($options['max_total_backoff_duration'])) { - $this->max_total_backoff_duration_ms = (int)$options['max_total_backoff_duration']; + $this->max_total_backoff_duration_ms = (int)$options['max_total_backoff_duration'] * 1000; } if (isset($options['max_rate_limit_duration'])) { - $this->max_rate_limit_duration_ms = (int)$options['max_rate_limit_duration']; + $this->max_rate_limit_duration_ms = (int)$options['max_rate_limit_duration'] * 1000; } - if (isset($options['rate_limit_retry_after_cap_s'])) { - $this->rate_limit_retry_after_cap_s = (int)$options['rate_limit_retry_after_cap_s']; + if (isset($options['rate_limit_retry_after_cap'])) { + $this->rate_limit_retry_after_cap_s = (int)$options['rate_limit_retry_after_cap']; } if (isset($options['retry_count'])) { diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 13f00db..20d6216 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -240,27 +240,61 @@ public function testRetryAfterOn429DoesNotDecrementRetries(): void } /** - * 429 + Retry-After: 3 → budget exhausted after retry_count retries on other codes. - * Re-verify: if retry_count is 1 and we get a 503 (no Retry-After), we fail immediately. + * 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. */ - public function testNon429ExhaustsRetryBudget(): void + public function testRetryCountGrantsExactlyThatManyRetries(): void { $consumer = new MockLibCurl('test-secret', ['retry_count' => 1]); $consumer->responses = [ [503, [], 'Service Unavailable', ''], - // retry_count=1 means retriesRemaining starts at 1, after one decrement it's 0 → return false + [503, [], 'Service Unavailable', ''], ]; $result = $consumer->flushBatch(makeTestMessages()); self::assertFalse($result); - // retry_count = 1, so the single decrement exhausts the budget and the - // batch is abandoned without ever waiting. + self::assertSame(1, $consumer->backoffSleeps, 'retry_count 1 should grant one retry'); + self::assertCount(1, $consumer->sleepCalls); + } + + public function testRetryCountOfZeroGrantsNoRetries(): void + { + $consumer = new MockLibCurl('test-secret', ['retry_count' => 0]); + + $consumer->responses = [ + [503, [], 'Service Unavailable', ''], + ]; + + self::assertFalse($consumer->flushBatch(makeTestMessages())); self::assertSame(0, $consumer->backoffSleeps); self::assertCount(0, $consumer->sleepCalls); } + public function testTransportErrorReportsTheRealCurlErrno(): void + { + // The refactor dropped curl_errno and passed a literal 0, so every transport + // failure looked identical to an error_handler branching on the code. + $reported = []; + $consumer = new MockLibCurl('test-secret', [ + 'error_handler' => function ($code, $msg) use (&$reported) { + $reported[] = [$code, $msg]; + }, + ]); + + // 28 is CURLE_OPERATION_TIMEDOUT. + $consumer->responses = [ + [0, [], '', 'Operation timed out after 5000 milliseconds', 28], + ]; + + self::assertFalse($consumer->flushBatch(makeTestMessages())); + self::assertCount(1, $reported); + self::assertSame(28, $reported[0][0]); + } + // ------------------------------------------------------------------------- // parseRetryAfter HTTP-date tests // ------------------------------------------------------------------------- @@ -346,7 +380,7 @@ public function testRetryAfterCapIsRespected(): void { $consumer = new MockLibCurl('test-secret', [ 'retry_count' => 3, - 'rate_limit_retry_after_cap_s' => 300, + 'rate_limit_retry_after_cap' => 300, ]); $consumer->responses = [ diff --git a/test/MockLibCurl.php b/test/MockLibCurl.php index 4af77ce..28c7b3d 100644 --- a/test/MockLibCurl.php +++ b/test/MockLibCurl.php @@ -32,10 +32,14 @@ public function __construct(string $secret, array $options = []) protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array { if (empty($this->responses)) { - return [200, [], '{"success":true}', '']; + return [200, [], '{"success":true}', '', 0]; } - return array_shift($this->responses); + $response = array_shift($this->responses); + + // Rows may omit the curl errno; default it so tests that do not care about + // transport errors stay as four-element arrays. + return $response + [4 => 0]; } /** From 17e5c7fa9f370e4044d874541c25d9393afce1da Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 11:17:31 -0400 Subject: [PATCH 14/16] Wire the retry budgets into Socket, reject self-contradicting dates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Socket consumer referenced none of the new retry-budget options, so setting retry_count or max_total_backoff_duration had no effect there: it gave up after a fixed seven retries over about 13 seconds regardless. It is a selectable consumer ('socket' in Client::$consumers), not a legacy path, so it now uses the same counted budget and total-duration budget as LibCurl, and backs off from 500ms rather than 100ms. maximum_backoff_duration changes role from ending the loop to capping each individual wait, so anyone who set it still gets a bound on how long one retry sleeps. max_rate_limit_duration stays inapplicable there, since Socket still cannot read Retry-After; the docstring says so. parseRetryAfter accepted HTTP-dates whose day name contradicts the date. createFromFormat does not validate that token: on a mismatch it silently rolls the result forward to the next matching weekday and reports no warning at all, so "Thu, 20 Sep 2026" — a Sunday — parsed as 24 Sep, turning a date four days in the past into one in the future and taking the rate-limit path, which spends no retry budget. My first attempt compared the parsed date's own weekday against the header and did nothing, because the roll-forward is precisely what makes those two agree. Re-formatting the whole value and comparing does catch it, with whitespace collapsed so asctime's double-spaced single-digit days still round-trip. There is a test for each of the three permitted formats guarding against over-rejection, and the mismatch test fails without the fix. 81 unit tests (the same 2 pre-existing failures), phpcs clean, and the 61-test e2e suite passes. --- HISTORY.md | 3 ++- lib/Consumer/QueueConsumer.php | 17 ++++++++++++++ lib/Consumer/Socket.php | 30 ++++++++++++++++++++----- test/ConsumerLibCurlTest.php | 41 ++++++++++++++++++++++++++++++++++ test/ConsumerSocketTest.php | 26 +++++++++++++++++++++ 5 files changed, 110 insertions(+), 7 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 2b5dbeb..3e5059f 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -13,7 +13,8 @@ sent the write key as HTTP Basic credentials. * 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. These apply to the LibCurl consumer; the Socket consumer stays in maintenance mode and still bounds retries with the older `maximum_backoff_duration` alone. + * 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. * 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. diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index cc0be2f..29a2af6 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -207,6 +207,18 @@ protected function parseRetryAfter(?string $value): ?int continue; } + // createFromFormat does not check the day-name token against the rest of + // the date: on a mismatch it silently rolls the result forward to the next + // matching weekday and reports no warning, so "Thu, 20 Sep 2026" — actually + // a Sunday — parses as 24 Sep, turning a date in the past into one in the + // future. Comparing the parsed date's own weekday cannot catch this, since + // the roll-forward is what makes the two agree; re-formatting the whole + // value and comparing does. Whitespace is collapsed so asctime's + // double-spaced single-digit days still round-trip. + if (strcasecmp(self::collapseWhitespace($date->format($format)), self::collapseWhitespace($value)) !== 0) { + continue; + } + $seconds = $date->getTimestamp() - time(); return $seconds > 0 ? $seconds : null; } @@ -214,6 +226,11 @@ protected function parseRetryAfter(?string $value): ?int return null; } + private static function collapseWhitespace(string $value): string + { + return trim((string)preg_replace('/\s+/', ' ', $value)); + } + /** * Tracks a user action * diff --git a/lib/Consumer/Socket.php b/lib/Consumer/Socket.php index 5afb08a..8bf94fb 100644 --- a/lib/Consumer/Socket.php +++ b/lib/Consumer/Socket.php @@ -147,7 +147,11 @@ private function createBody(string $host, string $content, int $attempt = 1) * - Status code classification: Full support (retryable vs non-retryable * per e2e spec, via parent isRetryable()). * - X-Retry-Count: Supported. - * - Backoff: Exponential with cap (maximum_backoff_duration). + * - 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_rate_limit_duration: not applicable, since there is no Retry-After + * path here for it to bound. * * For full Retry-After support, use the default LibCurl consumer. * @@ -163,8 +167,10 @@ private function makeRequest($socket, string $req, string $payload): bool $closed = false; // Retries with exponential backoff until success - $backoff = 100; // Set initial waiting time to 100ms - $attempt = 1; + $backoffMs = 500; // base 500ms, matching the LibCurl consumer + $retriesRemaining = $this->retry_count; + $backoffStartTime = null; + $attempt = 1; while (true) { // Send request to server @@ -202,12 +208,24 @@ private function makeRequest($socket, string $req, string $payload): bool return false; } - if ($backoff >= $this->maximum_backoff_duration) { + // Counted retries and the total-duration budget, shared with the LibCurl + // consumer. Retry-After is still not honoured here; see the note above. + if ($retriesRemaining <= 0) { break; } + $retriesRemaining--; - usleep($backoff * 1000); - $backoff *= 2; + if ($backoffStartTime === null) { + // hrtime is monotonic; microtime would let a clock adjustment expire + // or extend this budget. + $backoffStartTime = hrtime(true); + } + if ((hrtime(true) - $backoffStartTime) / 1e6 >= $this->max_total_backoff_duration_ms) { + break; + } + + usleep($backoffMs * 1000); + $backoffMs = min($backoffMs * 2, $this->maximum_backoff_duration); $attempt++; $socket = $this->createSocket(); diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 20d6216..8831d0b 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -274,6 +274,47 @@ public function testRetryCountOfZeroGrantsNoRetries(): void self::assertCount(0, $consumer->sleepCalls); } + public function testRejectsRetryAfterWhoseWeekdayContradictsTheDate(): void + { + // PHP's createFromFormat silently rolls a weekday/date mismatch forward to the + // next matching weekday and reports no warning, so this turned a date in the + // past into one ~4 days in the future and took the rate-limit path, which + // spends no retry budget. 20 Sep 2026 was a Sunday, not a Thursday. + $consumer = new MockLibCurl('test-secret'); + + self::assertNull($consumer->publicParseRetryAfter('Thu, 20 Sep 2026 10:49:58 GMT')); + } + + public function testAcceptsAllThreeRfc7231DateFormats(): void + { + // Guards the round-trip check added above against over-rejecting: asctime pads + // single-digit days with a second space, which a naive comparison would fail. + $consumer = new MockLibCurl('test-secret'); + $future = new \DateTimeImmutable('+2 hours'); + + self::assertSame( + 7200, + $consumer->publicParseRetryAfter($future->format('D, d M Y H:i:s') . ' GMT'), + 'IMF-fixdate' + ); + self::assertSame( + 7200, + $consumer->publicParseRetryAfter($future->format('l, d-M-y H:i:s') . ' GMT'), + 'RFC 850' + ); + self::assertSame( + 7200, + $consumer->publicParseRetryAfter(sprintf( + '%s %s %2d %s', + $future->format('D'), + $future->format('M'), + (int)$future->format('j'), + $future->format('H:i:s Y') + )), + 'asctime, double-spaced single-digit day' + ); + } + public function testTransportErrorReportsTheRealCurlErrno(): void { // The refactor dropped curl_errno and passed a literal 0, so every transport diff --git a/test/ConsumerSocketTest.php b/test/ConsumerSocketTest.php index f4096c7..1a5ccb1 100644 --- a/test/ConsumerSocketTest.php +++ b/test/ConsumerSocketTest.php @@ -8,6 +8,8 @@ use PHPUnit\Framework\TestCase; use RuntimeException; use Segment\Client; +use Segment\Consumer\QueueConsumer; +use Segment\Consumer\Socket; class ConsumerSocketTest extends TestCase { @@ -253,4 +255,28 @@ public function testRequestCompression(): void self::assertTrue($client->track(['user_id' => 'some-user', 'event' => 'Socket PHP Event'])); $client->__destruct(); } + + /** + * The Socket consumer referenced none of the shared retry-budget options, so + * setting them had no effect there at all — it gave up after a fixed ~13s + * regardless. It is a selectable consumer ('socket' in Client::$consumers), + * not a legacy path, so the options have to reach it. + */ + public function testSocketReadsTheSharedRetryBudgetOptions(): void + { + $consumer = new Socket('test-secret', [ + 'retry_count' => 7, + 'max_total_backoff_duration' => 120, + ]); + + $read = function (string $property) use ($consumer) { + $ref = new \ReflectionProperty(QueueConsumer::class, $property); + $ref->setAccessible(true); + return $ref->getValue($consumer); + }; + + self::assertSame(7, $read('retry_count')); + // Options are in seconds; the field behind them is milliseconds. + self::assertSame(120000, $read('max_total_backoff_duration_ms')); + } } From 8533b5a333de242cd1a92d26290d6085d300127b Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:20:14 -0400 Subject: [PATCH 15/16] Reject negative retry budgets instead of casting them in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (int)$options[...] was applied without checking, so a negative value silently disabled retrying: retriesRemaining started below zero and failed its first check, and a negative duration budget was already exceeded the first time it was compared. Someone setting these is asking for more retrying, not none. Bad values now log and keep the default, matching how flush_at and flush_interval are handled a few lines above rather than introducing an exception this constructor does not otherwise throw. Zero stays valid. analytics-python validates the same options as non-negative, and retry_count of 0 meaning "do not retry" is deliberate there and in analytics-ruby, so rejecting it here would have made php the odd one out — my first pass at this did exactly that and broke the retry_count-of-zero test, which is what caught it. 83 unit tests (the same 2 pre-existing failures), phpcs clean, 61-test e2e suite passes. --- HISTORY.md | 1 + lib/Consumer/QueueConsumer.php | 43 ++++++++++++++++++++++++++++++---- test/ConsumerLibCurlTest.php | 33 ++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 3e5059f..035731c 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -16,6 +16,7 @@ sent the write key as HTTP Basic credentials. * 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. diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 29a2af6..5c8c7c8 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -90,20 +90,34 @@ public function __construct(string $secret, array $options = []) // These three are in SECONDS, matching the options of the same names in the // python, ruby, go and java clients. The _ms fields behind them are internal; // taking milliseconds here made 43200 mean 43 seconds rather than 12 hours. + // + // Negatives are rejected rather than cast blindly: they silently disabled + // retrying altogether, the opposite of what someone setting these is asking + // for. Zero is allowed and meaningful — retry_count 0 means "do not retry", + // matching analytics-python and analytics-ruby. Bad values log and keep the + // default, the way flush_at and flush_interval above do. if (isset($options['max_total_backoff_duration'])) { - $this->max_total_backoff_duration_ms = (int)$options['max_total_backoff_duration'] * 1000; + if ($this->isNonNegativeInt($options['max_total_backoff_duration'], 'max_total_backoff_duration')) { + $this->max_total_backoff_duration_ms = (int)$options['max_total_backoff_duration'] * 1000; + } } if (isset($options['max_rate_limit_duration'])) { - $this->max_rate_limit_duration_ms = (int)$options['max_rate_limit_duration'] * 1000; + if ($this->isNonNegativeInt($options['max_rate_limit_duration'], 'max_rate_limit_duration')) { + $this->max_rate_limit_duration_ms = (int)$options['max_rate_limit_duration'] * 1000; + } } if (isset($options['rate_limit_retry_after_cap'])) { - $this->rate_limit_retry_after_cap_s = (int)$options['rate_limit_retry_after_cap']; + if ($this->isNonNegativeInt($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']; + } } if (isset($options['retry_count'])) { - $this->retry_count = (int)$options['retry_count']; + if ($this->isNonNegativeInt($options['retry_count'], 'retry_count')) { + $this->retry_count = (int)$options['retry_count']; + } } $this->queue = []; @@ -154,6 +168,27 @@ public function flush(): bool * 5xx are retryable except 501, 505, 511. * 4xx are non-retryable except 408, 410, 429, 460. */ + /** + * 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) { + error_log(sprintf( + '[Analytics][%s] %s must be a non-negative integer; keeping the default', + $this->type, + $name + )); + return false; + } + + return true; + } + protected function isRetryable(int $statusCode): bool { if ($statusCode >= 500 && $statusCode < 600) { diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 8831d0b..cd2e8d1 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -7,6 +7,7 @@ use PHPUnit\Framework\TestCase; use RuntimeException; use Segment\Client; +use Segment\Consumer\QueueConsumer; /** Minimal message fixture for flushBatch calls */ function makeTestMessages(): array @@ -274,6 +275,38 @@ public function testRetryCountOfZeroGrantsNoRetries(): void self::assertCount(0, $consumer->sleepCalls); } + 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. + $consumer = new MockLibCurl('test-secret', [ + 'retry_count' => -5, + 'max_total_backoff_duration' => -1, + ]); + + $read = function (string $property) use ($consumer) { + $ref = new \ReflectionProperty(QueueConsumer::class, $property); + $ref->setAccessible(true); + return $ref->getValue($consumer); + }; + + self::assertSame(10, $read('retry_count'), 'default retry_count'); + self::assertSame(43200000, $read('max_total_backoff_duration_ms'), 'default 12h budget'); + } + + public function testZeroRetryCountIsAcceptedRatherThanTreatedAsInvalid(): void + { + // retry_count 0 means "do not retry" and is deliberate in analytics-python + // and analytics-ruby, so php accepts it rather than falling back to 10. + $consumer = new MockLibCurl('test-secret', ['retry_count' => 0]); + + $ref = new \ReflectionProperty(QueueConsumer::class, 'retry_count'); + $ref->setAccessible(true); + + self::assertSame(0, $ref->getValue($consumer)); + } + public function testRejectsRetryAfterWhoseWeekdayContradictsTheDate(): void { // PHP's createFromFormat silently rolls a weekday/date mismatch forward to the From d3e11ca56b9f156bcc8076b3d724559cf62bf2cb Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:52:18 -0400 Subject: [PATCH 16/16] Reunite two docblocks with the methods they describe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were my own doing: inserting isNonNegativeInt and sleepBeforeRetry directly above an existing method put each new method between a docblock and the code it documented. The result was two stacked comments, the original orphaned above the newcomer, and isRetryable and executeHttpRequest left with none. executeHttpRequest's was also stale — it documented a 4-tuple return, but the tuple gained curlErrno when transport errors stopped reporting 0. Scanned the rest of lib/ for the same pattern; these two were the only instances. phpcs clean, 83 unit tests (the same 2 pre-existing failures). --- lib/Consumer/LibCurl.php | 24 ++++++++++++------------ lib/Consumer/QueueConsumer.php | 10 +++++----- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 8affe2f..eca02bd 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -122,18 +122,6 @@ public function flushBatch(array $messages): bool } } - /** - * Execute an HTTP POST request via cURL. - * - * Returns [statusCode, responseHeaders, responseBody, curlError]. - * responseHeaders keys are lower-cased. - * - * @param string $url - * @param string $secret - * @param string $payload - * @param array $headers - * @return array{int, array, string|false, string} - */ /** * Wait before the next attempt. Separate from flushBatch so tests can observe the * retry schedule by overriding this alone. @@ -146,6 +134,18 @@ protected function sleepBeforeRetry(int $milliseconds, bool $rateLimited): void usleep($milliseconds * 1000); } + /** + * Execute an HTTP POST request via cURL. + * + * Returns [statusCode, responseHeaders, responseBody, curlError, curlErrno]. + * responseHeaders keys are lower-cased. + * + * @param string $url + * @param string $secret + * @param string $payload + * @param array $headers + * @return array{int, array, string|false, string, int} + */ protected function executeHttpRequest(string $url, string $secret, string $payload, array $headers): array { $responseHeaders = []; diff --git a/lib/Consumer/QueueConsumer.php b/lib/Consumer/QueueConsumer.php index 5c8c7c8..281d49c 100644 --- a/lib/Consumer/QueueConsumer.php +++ b/lib/Consumer/QueueConsumer.php @@ -163,11 +163,6 @@ public function flush(): bool return $success; } - /** - * Determine if a status code is retryable per e2e spec. - * 5xx are retryable except 501, 505, 511. - * 4xx are non-retryable except 408, 410, 429, 460. - */ /** * Whether an option value is usable as a count or duration. * @@ -189,6 +184,11 @@ protected function isNonNegativeInt($value, string $name): bool return true; } + /** + * Determine if a status code is retryable per e2e spec. + * 5xx are retryable except 501, 505, 511. + * 4xx are non-retryable except 408, 410, 429, 460. + */ protected function isRetryable(int $statusCode): bool { if ($statusCode >= 500 && $statusCode < 600) {