Handle Retry-After on every retryable status, including 529 - #148
Open
MichaelGHSeg wants to merge 4 commits into
Open
Handle Retry-After on every retryable status, including 529#148MichaelGHSeg wants to merge 4 commits into
MichaelGHSeg wants to merge 4 commits into
Conversation
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.
2 tasks
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled, which was too broad: a 500 with no Retry-After and backoff disabled was also kept, so the file was re-uploaded even though nothing had scheduled a retry. The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects exactly one request and saw two. ShouldDeleteBatch now takes the same retryAfterSeconds value handed to HandleResponse, so the two agree on whether the response actually took the rate-limit path. A retryable status keeps its batch only when it carries a usable Retry-After and rate limiting is on; otherwise only backoff can retry it, and with backoff off the batch is dropped as before. The single-argument overload is retained. 232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but IsSuccessStatusCode and the two status checks in RetryStateMachine were 2xx-only, so a 3xx fell through to the retry classifier. analytics-go, analytics-python and analytics-php already follow the spec here; this brings C# into line with them and with its own plan. 236 tests pass, including new cases covering 200, 201, 301 and 304.
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.
Stacked on #147 — review that first; this PR's diff is the single 529 commit.
Why
We were asked to add 529 support with
Retry-Afterheaders. The conclusion was to checkRetry-Afteron every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.This is the same change already shipped in analytics-java 3.5.5, and matches the
generic-retry-afterconformance suite merged in sdk-e2e-tests#16.What
Retry-Afterroutes through the rate-limit path, which does not consume retry budget.Retry-Aftercontinue to use counted exponential backoff.RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).Testing
Retry-After, and clamping tomaxRetryInterval.generic-retry-after(529) suite and all fourretry-settingssuites.E2E was run locally because CI cannot currently check out the private
sdk-e2e-testsrepo — thetoken: ${{ secrets.E2E_TESTS_TOKEN }}line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo'sci/harden-build-publishbranch and in go/ruby/php.Related
The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.