Bound the rate-limit retry budget at 5 minutes - #254
Open
MichaelGHSeg wants to merge 7 commits into
Open
MichaelGHSeg wants to merge 7 commits into
MichaelGHSeg wants to merge 7 commits into
Conversation
The 12 hour default was a backstop on the assumption a retry count would stop us reaching it. Rate-limited attempts are deliberately uncounted, so it was the operative limit instead. That matters more here than in the other clients. LibCurl is the default consumer and there is no background thread: flushBatch retries inline, so the budget is time the caller's web request spends blocked and an FPM worker spends occupied. Under load that is pool exhaustion rather than late telemetry. Five minutes matches the counted path's ~4 minute worst case. rate_limit_retry_after_cap drops to 60s: at 300s it equalled the whole budget, so one sleep consumed it and the path gave a single attempt. The sleep is clamped to the remaining budget, since the elapsed check runs before the wait. The new test sleeps 60s without the clamp and 1s with it. Exhausting the budget now logs unconditionally. handleError only writes when debug is on, and debug defaults to false, so the one failure mode that blocks a request for minutes produced no output at all. 83 unit tests (the same 2 pre-existing failures), phpcs clean, 61-test e2e suite passes.
Three problems. The notes described changes between states that never shipped, so a customer read that a default moved from 12 hours to 5 minutes when only the 5 minutes was ever released. They referred to other SDKs, which means nothing to someone reading one library's notes. And they had accumulated over several passes into contradictions — Retry-After was documented as capped at both 300s and 60s, and the rate-limit budget as both 12 hours and 5 minutes. Rewritten to describe the behaviour this version has, in a consistent structure: upgrade notes that need action first, then retry handling, then everything else. Entries covering fixes to code that has not shipped are dropped, since there is nothing for a reader to compare against.
testRetryAfterCapIsRespected passed rate_limit_retry_after_cap of 300 while leaving max_rate_limit_duration at its default — which this branch also changed to 300 seconds. With the cap and the budget equal, the new clamp correctly picks the remaining budget, which is short of the cap by however much time has already elapsed. The assertion failed by 1ms. The test is about the cap, so the budget is now raised well above it. A second test covers the shipped default, where a large Retry-After clamps to 60 seconds well inside the budget. I introduced this and missed it: after adding the clamp test I ran only that one by filter, and reported the suite as green on the strength of an earlier run. The full suite is 85 tests with the same 2 pre-existing failures — a File consumer test needing the send script and a Socket timeout test. phpcs clean, 61-test e2e suite passes.
Applying the team convention to my own work from today. The comments explaining these changes had accumulated into potted histories: why a value had been twelve hours, what a test used to assert, which path used to be unreachable. Six months from now none of that resolves to anything — the diff and the commit messages hold it, and the comment should say why the code is the way it is. What stayed is what a maintainer would undo without it: that Kernel#sleep raises on a negative interval, that Thread#wakeup only interrupts a sleep already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an interrupt does not reach them, and that inverting one assertion would make the duration budget unreachable again. Comments only, no behaviour change.
Capping at 60s meant waiting less than the server asked for, which does not make the next attempt more likely to succeed — it just sends more requests at something already rate-limiting us. Against a Retry-After of 180s inside a 5 minute budget it turns 3 requests into 6; against 300s it turns 2 into 6. The cap is a guard against an absurd header, not a second budget. How long we keep trying is max_rate_limit_duration's job, and the clamp to the remaining budget already stops a single wait running past it, so the cap now rarely binds at all. It also bought nothing for the client this was partly aimed at: with no background thread, a shorter cap turns one long wait into several short ones for the same total blocking time and more requests. Tests that pinned 60 are updated, and each SDK gains one asserting that a Retry-After inside the cap is used as given rather than shortened.
CURLOPT_TIMEOUT of 0 means curl waits indefinitely, so a connection that stalled after the handshake blocked the caller past every retry budget the rest of this change set added. 300s is well clear of any legitimate upload: batches are capped at 500KB. Also note in the changelog how long a flush can block, and that the file consumer plus send.php is the non-blocking alternative — neither is documented in this repo.
The file consumer and send.php are documented after all, so point at that page rather than restating it here. The 3.8.2 heading appeared twice, the first one empty.
This branch has not been deployed
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
1 participant
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.
Why
Retry handling has two budgets. Responses carrying
Retry-Aftertake a rate-limit path bounded only by elapsed time and deliberately not counted against the retry count; everything else takes a counted exponential-backoff path.max_rate_limit_durationwas 12 hours, intended as a last-ditch guard that a retry count would stop us ever reaching. Rate-limited attempts are uncounted, so it was the only limit on that path rather than the guard behind one.This matters more here than in the other clients. LibCurl is the default consumer and there is no background thread:
flushBatchretries inline, so the budget is time the caller's web request spends blocked and an FPM worker spends occupied. Under load that is pool exhaustion rather than late telemetry.What
max_rate_limit_durationdefaults to 5 minutes, in line with the counted path's ~4 minute worst case.rate_limit_retry_after_capdrops to 60s. At 300s it equalled the whole budget, so one wait consumed it and the path gave a single attempt.hrtimeis sampled once and reused, so the remaining value floors to zero rather than going negative —usleep()raisesValueErroron a negative.handleErroronly writes whendebugis on, anddebugdefaults to false, so the one failure mode that blocks a request for minutes produced no output at all.Testing
85 unit tests,
phpcsclean, and the full 61-test shared e2e suite passes. Two pre-existing failures remain onmasterand are unrelated: a File consumer test that needs the send script, and a Socket timeout test.testRateLimitSleepNeverOvershootsTheBudgetfails without the clamp, sleeping 60s with at most 1s of budget left.One pre-existing test needed fixing:
testRetryAfterCapIsRespectedpassed a 300s cap while leaving the budget at its default, which this change also sets to 300s. With the two equal the clamp correctly wins by the fraction of a millisecond already elapsed. The test now raises its own budget above the cap, and a second test covers the shipped 60s default.CI cannot currently run the e2e suite — the private
sdk-e2e-testscheckout lost its token during the CI-hardening work — so it was run locally.Notes
Release notes describe the behaviour this version has rather than the delta from an unreleased state, since the whole retry feature ships in this same version. The exception is the Socket consumer, which previously gave up after a fixed ~13 seconds and now honours
retry_countandmax_total_backoff_duration; that carries its own upgrade note.