Unified HTTP response handling and retry behavior, enabled by default - #147
Merged
Merged
Conversation
The retry state machine added in #144 could only ever be configured from CDN settings: RateLimitConfig, BackoffConfig and HttpConfig were all internal and Configuration had no entry point, so a C# consumer could not set retry behaviour at all. Kotlin and Swift both expose this. Kotlin has `Configuration.httpConfig: HttpConfig?` with a public `data class HttpConfig`; Swift has `public func httpConfig(_ config: HttpConfig?) -> Configuration`. This brings C# in line with the SDKs #144 was written to match. - Make RetryBehavior, RateLimitConfig, BackoffConfig and HttpConfig public. RetryConfig stays internal — it is plumbing built from HttpConfig, never supplied by callers. - Add Configuration.HttpConfig, as a trailing optional constructor argument so existing positional callers are unaffected. Defaults to null, preserving today's CDN-only behaviour. - Have EventPipelineProvider and SyncEventPipelineProvider pass it through as the pipeline's starting retry config. CDN settings still override it later via UpdateHttpConfig. - Make the pipeline constructors that take an HttpConfig public, so a custom IEventPipelineProvider can pass one on rather than only read it. 216 tests pass, including 6 new ones covering that a config set on Configuration reaches both pipelines' retry state machines.
This was referenced Sep 3, 2026
Merged
Two problems that only matter once these types are public: - BackoffConfig stored a reference to the shared static DefaultStatusCodeOverrides whenever no map was supplied. With StatusCodeOverrides exposed as a public property, a caller doing the natural thing — cfg.StatusCodeOverrides[500] = Drop — corrupted the defaults for every BackoffConfig constructed afterwards in the process, including ones parsed from CDN settings, with no way to reset. The constructor now copies the map. - A user-supplied HttpConfig reached the retry state machine unclamped, while the CDN path is validated by HttpConfigParser. Configuration.HttpConfig was therefore the only unvalidated route in, so out-of-range values such as maxRetryInterval: 0 or a negative jitterPercent took effect verbatim. Both pipelines now call Validated() on user-supplied config, matching the CDN path. 218 tests pass, including two new cases covering the copy and the clamping.
The property doc said retry settings come from CDN settings alone when this is null, which reads as 'non-null means yours is used'. It is not: SegmentDestination calls UpdateHttpConfig on every settings refresh carrying an httpConfig key, which replaces the whole config. A CDN payload also counts as enabling a subsystem unless it explicitly says enabled: false, so a payload tuning something unrelated can turn retries back on. Only a payload with no httpConfig key leaves this value in effect. This matches analytics-kotlin (SegmentDestination.kt:133) and analytics-swift (SegmentDestination.swift:83-91), which assign CDN config over the user's the same way and share the enabled-defaults-true rule, so the behaviour is left alone and only the documentation is corrected.
Adding a trailing optional parameter to Configuration's constructor is source compatible but not binary compatible: the compiler bakes optional defaults into the call site, so the assembly loses the old 13-parameter .ctor and anything compiled against it fails with MissingMethodException. That is fine for NuGet consumers, who recompile, but this SDK also ships Unity and Xamarin samples where DLLs are dropped in. #144 never touched Configuration.cs, so the break would have been new here. Making HttpConfig a settable property is purely additive, leaves the existing constructor signature untouched, and is closer to analytics-kotlin, which uses a mutable 'var httpConfig' rather than a constructor argument. new Configuration("writeKey") { HttpConfig = new HttpConfig(...) } 218 tests pass.
Cut the before/after narration from the comments added with the HttpConfig work. The copy of StatusCodeOverrides and the Validated() calls now state why they are needed rather than what the code did without them.
wenxi-zeng
previously approved these changes
Sep 21, 2026
…eneric Retry-After (529) (#148) * 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. * Keep the batch when Retry-After routes it to the rate-limit path 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. * Base the keep-or-delete decision on Retry-After, not just config 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. * Treat 3xx as success, per spec item 1 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. * Send the Authorization header, and drop 511 Two Key Agreements from the HTTP response design doc that this SDK did not meet. The doc requires every SDK to send the write key in the Authorization header, and TAPI authenticates and routes on it instead of parsing the payload — which is the performance reason the header exists. This SDK sent no Authorization at all; it relied solely on the writeKey embedded in the batch body by Storage. Upload requests now carry Basic credentials built from the write key with an empty password, matching analytics-python, -go, -ruby, -php and -java, all of which send base64("<writeKey>:"). The value is exposed as a protected BasicAuthorization on HTTPClient rather than by widening _apiKey, so a custom IHTTPClientProvider can send the same header; the Unity sample, which overrides DoPost, now does. The writeKey stays in the payload, so nothing depends on the header alone yet. Separately, 511 Network Authentication Required was retryable here. The doc makes it conditional — "Authenticate, then retry if library supports OAuth" — and this SDK has no OAuth, so a 511 could never be satisfied and retrying only spent the budget. It joins 501 and 505 as an explicit Drop. analytics-python, the one SDK with OAuth, correctly retries 511 only when an OauthManager is configured; go, ruby, php and java exclude it as this now does. 240 tests pass, including new coverage of the header value, and all 79 e2e tests still pass. * 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.
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. HttpClient follows what it can. Three sites narrowed, and the tests now assert 300/301/304 are not success rather than that they are.
ShouldUploadBatch compares a fresh state's counts against MaxRetryCount before the first attempt: RetryStateMachine.cs:92 checks GlobalRetryCount, which starts at 0. Validated() clamped with Math.Max(0, ...), so maxRetryCount: 0 — a plausible way to say "do not retry" — made that 0 >= 0 and dropped every batch before it was ever sent, not merely after a failure. Exposing HttpConfig publicly made that reachable from user code as well as from CDN settings, so both clamps now floor at 1. analytics-kotlin and analytics-swift clamp the same way and have the same hole. 251 tests pass, including one that asserts a fresh batch proceeds under maxRetryCount: 0; it fails against the old clamp. All 82 e2e tests pass.
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.
Both retry subsystems defaulted to disabled, and Configuration.HttpConfig defaults to null, which funnels into the same constructors. Server-side deployments get no CDN settings to turn them on, so they retried nothing: 408/410/460 dropped, Retry-After ignored, 429 and 5xx held with no delay and no budget. Authorization and server load are the point of this initiative, so shipping it inert for server users defeats it. Enabling it unchanged would have been worse than leaving it off: the enabled-mode defaults were 100 retries with a 300s ceiling, against 10 and 60s everywhere else, so every C# client would have hit the endpoint an order of magnitude harder than a go or python one. Both now match. Mobile is unaffected. SegmentDestination.Update only touches the config when a settings payload actually carries an httpConfig key, and the parser passes `enabled` explicitly on every path, so CDN settings still win and a payload without that key leaves the local config in effect. Also: - ShouldDeleteBatch now reads the per-cycle snapshot like every other call in the upload loop. Reading the live volatile field let a CDN refresh mid-upload delete a batch that HandleResponse had just scheduled a retry for. - HttpConfigParser reads its fallbacks from the config types instead of keeping its own copies, which had already drifted from them. - The legacy one-arg Upload path is pinned to a disabled config so its drop/keep behaviour does not move with the new default. - Records why a 429 is dropped when rate limiting is off: it is a kill switch symmetric with backoffConfig.enabled:false, asserted by the shared e2e suite. I tried to "fix" it to fall through to backoff and the conformance test correctly caught it. 254 unit tests and the full 82-test e2e suite pass.
This repo writes "behavior" 75 times to "behaviour" twice, and both outliers were mine.
MichaelGHSeg
changed the title
Every other SDK bounds the rate-limit path by elapsed time — java, go, python, ruby and php all use a 12h budget and none of them caps it by a count. C# had only the count. I checked all five: there is no lower retry count limit elsewhere to match, so this adds the duration budget as the last-ditch guard and leaves the count as what actually stops retrying. At the defaults the count is reached first by a wide margin: 100 retries against a 300s ceiling is 8.3h against a 12h budget. There is a test asserting that relationship rather than the two numbers, so the duration cannot quietly become the operative limit. RetryState carries a RateLimitStartTime, stamped on the first rate-limited response of an episode and cleared on the first success. It is persisted alongside waitUntilTime; a stored state written before this change has no such key and reads back as null, so existing files still load. Retry-After was configurable up to 3600s where the other SDKs fix the ceiling at 300s. Now capped at 300s, via a named constant so the test can assert against it rather than restating the number. EventPipeline and SyncEventPipeline constructors go back to internal. Tests and e2e-cli both have InternalsVisibleTo, so nothing needed them public, and widening them sat badly next to keeping RetryConfig internal as plumbing. Left alone deliberately: Retry-After: 0 on a 429 retries immediately while an absent header waits the full ceiling. Either behaviour is defensible and the corner case is narrow. 259 unit tests and the full 82-test e2e suite pass.
BackoffConfig took `statusCodeOverrides ?? DefaultStatusCodeOverrides`, so supplying an override for a single status discarded the defaults for every other one. Overriding 503 alone stopped 408, 410, 429 and 460 being retried, and dropped 511 out of the table so it fell through to Default5xxBehavior and started being retried — the one thing 511 must never do, since this library cannot re-authenticate. The same applied to CDN settings: a payload whose statusCodeOverrides were all unparseable produced an empty table and wiped the defaults. The test covering that asserted the dictionary came back empty, so it encoded the behaviour rather than catching it; it now checks that the junk is dropped and the defaults survive. Overrides still win where they overlap, so nothing becomes unopposable — a caller can still force 429 to Drop. Separately, MaxTotalBackoffDuration is floored at 1 second, for the same reason maxRetryCount is floored at 1: ExceedsMaxDuration compares elapsed time against it, so 0 meant "no budget" and abandoned the batch on its second attempt rather than meaning "no cap". 262 unit tests and the 82-test e2e suite pass.
The field was serialized but never round-tripped in a test, and it is the one piece of retry state added late. Two cases: it survives save/load, and state written before the key existed loads as null rather than the epoch, which would read as an episode that began in 1970 and expire every batch on sight. 264 unit tests pass.
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.
Why
This completes the HTTP response handling and retry work for C#, bringing it in line with the TAPI HTTP Key Agreements and with the other Segment SDKs (
analytics-javashipped the equivalent in 3.5.5).It started as just the user-facing entry point: the retry state machine added in #144 could only be configured from CDN settings, since
RateLimitConfig,BackoffConfigandHttpConfigwere allinternalandConfigurationhad no way in. It has since absorbed #148 and the findings from two review passes.Behavior change: retries are on by default
Through 2.6.0, both retry subsystems defaulted to disabled and
Configuration.HttpConfigdefaults tonull, which funnels into the same constructors. Server-side deployments receive no CDN settings, so in practice they retried nothing: 408, 410 and 460 were dropped,Retry-Afterwas ignored, and a 429 or 5xx was held with no delay and no budget. Since the Authorization header and server load are the point of this initiative, shipping it inert for server users defeats it.Both subsystems now default to enabled. To keep the old behavior, disable both explicitly:
Mobile is unaffected.
SegmentDestination.Updateonly callsUpdateHttpConfigwhen a settings payload actually carries anhttpConfigkey, andHttpConfigParserpassesenabledexplicitly on every path — so CDN settings still take precedence, and a payload without that key leaves the local config in effect. This matches analytics-kotlin and analytics-swift.Enabling it unchanged would have been worse than leaving it off: the enabled-mode defaults were 100 retries with a 300s ceiling, against 10 and 60s in java, go, python, ruby and php. Every C# client would have hit the endpoint an order of magnitude harder than a go or python one, so those two constants are corrected in the same change.
What
TAPI agreements (from #148)
Authorization: Basic base64("<writeKey>:")header. It remains in the request body, so no server-side change is needed.Retry-Afteron every retryable status rather than 429 alone, which brings 529 in through the generic 5xx rule. Numeric seconds and RFC 7231 HTTP-date are both accepted, clamped toMaxRetryInterval.RetryBehavior.Drop: it asks the client to re-authenticate, which this library cannot do, so retrying would spend the budget on a request that can never succeed.Configuration surface
RetryBehavior,RateLimitConfig,BackoffConfigandHttpConfigpublic.RetryConfigstaysinternal— it is plumbing built fromHttpConfig, never supplied by callers.Configuration.HttpConfigas a settable property, mirroring analytics-kotlin's mutableConfiguration.httpConfig.EventPipelineProvider/SyncEventPipelineProviderpass it through as the pipeline's starting retry config.Correctness fixes found in review
ShouldDeleteBatchnow reads the per-cycleRetryStateMachinesnapshot like every other call in the upload loop. Reading the livevolatilefield let a CDN settings refresh landing mid-upload delete a batch thatHandleResponsehad just scheduled a retry for.RateLimitConfig.MaxRetryCountandBackoffConfig.MaxRetryCountare floored at 1.ShouldUploadBatchcompares a fresh state's count against the max, so a configured 0 dropped every batch without ever sending it.BackoffConfig.StatusCodeOverridesis copied rather than aliased, so a caller mutating the dictionary it passed in no longer changes a live config.HttpConfigParserreads its fallbacks from the config types instead of keeping its own copies, which had already drifted from them.HTTPClient.Uploadpath is pinned to a disabled config, so its drop/keep behavior does not move with the new default.Not changed, deliberately
rateLimitConfig.enabled: falsedrops a 429 rather than handing it to counted backoff. That looks like data loss, and I changed it — the shared e2e suite correctly failed, because it is symmetric withbackoffConfig.enabled: falsedropping a 500: each flag is the kill switch for its own status class (retry-settings/settings-enabled-flag). Reverted, with the reasoning recorded at both call sites so it does not get "fixed" again.Testing
254 unit tests and the full 82-test e2e suite pass locally, including the
retry-settingssuites. E2E cannot run in CI at present — the privatesdk-e2e-testscheckout lost its token in the CI-hardening work.Notes
Configurationportion of Dual-path retry: exponential backoff + CDN-driven httpConfig support #143 (closed), which added individualMaxRetries/MaxTotalBackoffDuration/MaxRateLimitDurationknobs. That shape matches analytics-python but not Kotlin/Swift, which Implement retry state machine aligned with Kotlin/Swift #144 was written to match.<Version>still needs bumping off the published 2.6.0 before tagging.CHANGELOG.mdis new in this PR and carries the upgrade notes that auto-generated release notes cannot.analytics-nextchange, so theAuthorizationandX-Retry-Countaddition is called out at the top ofCHANGELOG.md.