Dual-path retry: exponential backoff + rate-limit handling - #251
Merged
Merged
Conversation
- 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)
- 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
bsneed
previously approved these changes
May 28, 2026
didiergarcia
previously approved these changes
May 28, 2026
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.
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.
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.
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.
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.
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.
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.
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.
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.
…tions
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.
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.
(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.
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).
didiergarcia
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rewrites
LibCurl::flushBatch()with a structured dual-path retry system.rate_limit_retry_after_cap_s, default 300s), does NOT consume the retry budget. Bounded bymax_rate_limit_duration_ms(default 12h).retry_countandmax_total_backoff_duration_ms(default 12h).X-Retry-Countheader on retry attempts.array_spliceordering inQueueConsumer::flush()— batch is removed from the queue before callingflushBatch(), which handles all retries internally. This prevents__destruct()from re-flushing batches that already exhausted their retry budget.enqueue()/flush()return values only (not theerror_handlercallback, which fires for transient per-attempt errors too). WiresmaxRetriesfrom input config.retrytest suite.Test plan
./vendor/bin/phpunit --no-coveragepassesbasic,retrysuites pass (48/48)