From e30087ec45fc3281a4aa15306048bc971ec5b778 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 11:03:13 -0400 Subject: [PATCH 01/15] Expose HttpConfig so retry behaviour is user-configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Segment/Analytics/Configuration.cs | 12 ++- .../Segment/Analytics/Retry/RetryConfig.cs | 6 +- .../Segment/Analytics/Retry/RetryTypes.cs | 2 +- .../Analytics/Utilities/EventPipeline.cs | 2 +- .../Utilities/EventPipelineProvider.cs | 3 +- .../Analytics/Utilities/SyncEventPipeline.cs | 2 +- .../Utilities/SyncEventPipelineProvider.cs | 3 +- Tests/Retry/ConfigurationHttpConfigTest.cs | 99 +++++++++++++++++++ 8 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 Tests/Retry/ConfigurationHttpConfigTest.cs diff --git a/Analytics-CSharp/Segment/Analytics/Configuration.cs b/Analytics-CSharp/Segment/Analytics/Configuration.cs index 79de436..b4ed621 100644 --- a/Analytics-CSharp/Segment/Analytics/Configuration.cs +++ b/Analytics-CSharp/Segment/Analytics/Configuration.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using Segment.Analytics.Policies; +using Segment.Analytics.Retry; using Segment.Analytics.Utilities; using Segment.Concurrent; using Segment.Serialization; @@ -47,6 +48,12 @@ private set public IEventPipelineProvider EventPipelineProvider { get; } + /// + /// HTTP retry configuration for rate limiting and exponential backoff. + /// Defaults to null, meaning retry settings come from CDN settings alone. + /// + public HttpConfig HttpConfig { get; } + /// /// Configuration that analytics can use /// @@ -73,6 +80,7 @@ private set /// defaults to DefaultHTTPClientProvider /// /// set custom flush policies to tell analytics when and how to flush. If a value is given, it overwrites flushAt and flushInterval + /// retry configuration for rate limiting and exponential backoff. CDN settings, when present, take precedence public Configuration(string writeKey, int flushAt = 20, int flushInterval = 30, @@ -85,7 +93,8 @@ public Configuration(string writeKey, IStorageProvider storageProvider = default, IHTTPClientProvider httpClientProvider = default, IList flushPolicies = default, - IEventPipelineProvider eventPipelineProvider = default) + IEventPipelineProvider eventPipelineProvider = default, + HttpConfig httpConfig = null) { WriteKey = writeKey; FlushAt = flushAt; @@ -102,6 +111,7 @@ public Configuration(string writeKey, FlushPolicies.Add(new CountFlushPolicy(flushAt)); FlushPolicies.Add(new FrequencyFlushPolicy(flushInterval * 1000L)); EventPipelineProvider = eventPipelineProvider ?? new EventPipelineProvider(); + HttpConfig = httpConfig; } public Configuration(string writeKey, diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 5be1331..4e6463a 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -3,7 +3,7 @@ namespace Segment.Analytics.Retry { - internal class RateLimitConfig + public class RateLimitConfig { public bool Enabled { get; } public int MaxRetryCount { get; } @@ -23,7 +23,7 @@ public RateLimitConfig(bool enabled = false, int maxRetryCount = 100, int maxRet ); } - internal class BackoffConfig + public class BackoffConfig { public bool Enabled { get; } public int MaxRetryCount { get; } @@ -109,7 +109,7 @@ public RetryConfig(RateLimitConfig rateLimitConfig = null, BackoffConfig backoff } } - internal class HttpConfig + public class HttpConfig { public RateLimitConfig RateLimitConfig { get; } public BackoffConfig BackoffConfig { get; } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryTypes.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryTypes.cs index 7a87348..5884cb3 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryTypes.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryTypes.cs @@ -6,7 +6,7 @@ internal enum PipelineState RateLimited } - internal enum RetryBehavior + public enum RetryBehavior { Retry, Drop diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index 3056e8a..f35cecf 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -48,7 +48,7 @@ public EventPipeline( string apiHost = HTTPClient.DefaultAPIHost) : this(analytics, logTag, apiKey, flushPolicies, apiHost, (HttpConfig)null) { } - internal EventPipeline( + public EventPipeline( Analytics analytics, string logTag, string apiKey, diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipelineProvider.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipelineProvider.cs index abd376c..137780c 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipelineProvider.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipelineProvider.cs @@ -11,7 +11,8 @@ public IEventPipeline Create(Analytics analytics, string key) return new EventPipeline(analytics, key, analytics.Configuration.WriteKey, analytics.Configuration.FlushPolicies, - analytics.Configuration.ApiHost); + analytics.Configuration.ApiHost, + analytics.Configuration.HttpConfig); } } } \ No newline at end of file diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index 4657be9..c17443e 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -61,7 +61,7 @@ public SyncEventPipeline( CancellationToken? flushCancellationToken = null) : this(analytics, logTag, apiKey, flushPolicies, apiHost, flushTimeout, flushCancellationToken, null) { } - internal SyncEventPipeline( + public SyncEventPipeline( Analytics analytics, string logTag, string apiKey, diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipelineProvider.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipelineProvider.cs index 5794677..931a10b 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipelineProvider.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipelineProvider.cs @@ -22,7 +22,8 @@ public IEventPipeline Create(Analytics analytics, string key) analytics.Configuration.FlushPolicies, analytics.Configuration.ApiHost, _flushTimeout, - _flushCancellationToken); + _flushCancellationToken, + analytics.Configuration.HttpConfig); } } } \ No newline at end of file diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs new file mode 100644 index 0000000..26ff94e --- /dev/null +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -0,0 +1,99 @@ +using Moq; +using Segment.Analytics; +using Segment.Analytics.Retry; +using Segment.Analytics.Utilities; +using Segment.Serialization; +using Tests.Utils; +using Xunit; + +namespace Tests.Retry +{ + /// + /// Configuration.HttpConfig is the user-facing entry point for retry settings, + /// mirroring Kotlin's Configuration.httpConfig and Swift's .httpConfig(_:). + /// These cover that a config supplied there actually reaches the pipeline's + /// retry state machine; CDN settings still override it later via UpdateHttpConfig. + /// + public class ConfigurationHttpConfigTest + { + private static Analytics CreateAnalytics(HttpConfig httpConfig) + { + Settings? settings = JsonUtility.FromJson( + "{\"integrations\":{\"Segment.io\":{\"apiKey\":\"k\"}},\"plan\":{},\"edgeFunction\":{}}"); + + var mockHttpClient = new Mock(null, null, null); + mockHttpClient.Setup(c => c.Settings()).ReturnsAsync(settings); + + var config = new Configuration( + writeKey: "123", + autoAddSegmentDestination: false, + useSynchronizeDispatcher: true, + flushInterval: 0, + flushAt: 2, + httpClientProvider: new MockHttpClientProvider(mockHttpClient), + storageProvider: new MockStorageProvider(new Mock()), + httpConfig: httpConfig + ); + return new Analytics(config); + } + + [Fact] + public void Configuration_ExposesHttpConfig() + { + var httpConfig = new HttpConfig(backoffConfig: new BackoffConfig(enabled: true, maxRetryCount: 7)); + Analytics analytics = CreateAnalytics(httpConfig); + + Assert.Same(httpConfig, analytics.Configuration.HttpConfig); + } + + [Fact] + public void Configuration_HttpConfigDefaultsToNull() + { + Analytics analytics = CreateAnalytics(null); + + Assert.Null(analytics.Configuration.HttpConfig); + } + + [Fact] + public void EventPipeline_WithoutHttpConfig_IsLegacyMode() + { + Analytics analytics = CreateAnalytics(null); + + var pipeline = (EventPipeline)new EventPipelineProvider().Create(analytics, "key"); + + Assert.True(pipeline._retryStateMachine.IsLegacyMode); + } + + [Fact] + public void EventPipeline_WithHttpConfig_LeavesLegacyMode() + { + Analytics analytics = CreateAnalytics( + new HttpConfig(backoffConfig: new BackoffConfig(enabled: true))); + + var pipeline = (EventPipeline)new EventPipelineProvider().Create(analytics, "key"); + + Assert.False(pipeline._retryStateMachine.IsLegacyMode); + } + + [Fact] + public void SyncEventPipeline_WithoutHttpConfig_IsLegacyMode() + { + Analytics analytics = CreateAnalytics(null); + + var pipeline = (SyncEventPipeline)new SyncEventPipelineProvider().Create(analytics, "key"); + + Assert.True(pipeline._retryStateMachine.IsLegacyMode); + } + + [Fact] + public void SyncEventPipeline_WithHttpConfig_LeavesLegacyMode() + { + Analytics analytics = CreateAnalytics( + new HttpConfig(rateLimitConfig: new RateLimitConfig(enabled: true))); + + var pipeline = (SyncEventPipeline)new SyncEventPipelineProvider().Create(analytics, "key"); + + Assert.False(pipeline._retryStateMachine.IsLegacyMode); + } + } +} From d5bf3882ad381e8d8145b144c72df6c46c10fe60 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 13:07:11 -0400 Subject: [PATCH 02/15] Harden the newly public config surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Segment/Analytics/Retry/RetryConfig.cs | 5 +++- .../Analytics/Utilities/EventPipeline.cs | 6 ++-- .../Analytics/Utilities/SyncEventPipeline.cs | 6 ++-- Tests/Retry/ConfigurationHttpConfigTest.cs | 29 +++++++++++++++++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 4e6463a..3f83581 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -57,7 +57,10 @@ public BackoffConfig( Default4xxBehavior = default4xxBehavior; Default5xxBehavior = default5xxBehavior; UnknownCodeBehavior = unknownCodeBehavior; - StatusCodeOverrides = statusCodeOverrides ?? DefaultStatusCodeOverrides; + // Copy: the property is public, and sharing the static default would let one + // caller's mutation corrupt every BackoffConfig built afterwards in the process. + StatusCodeOverrides = new Dictionary( + statusCodeOverrides ?? DefaultStatusCodeOverrides); } public BackoffConfig Validated() => new BackoffConfig( diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index f35cecf..98c14bd 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -69,7 +69,9 @@ public EventPipeline( Running = false; var retryConfig = httpConfig != null - ? new RetryConfig(httpConfig.RateLimitConfig, httpConfig.BackoffConfig) + // Validated(): user-supplied config reaches us unclamped, unlike the + // CDN path which HttpConfigParser already validates. + ? new RetryConfig(httpConfig.RateLimitConfig.Validated(), httpConfig.BackoffConfig.Validated()) : new RetryConfig(); _retryStateMachine = new RetryStateMachine(retryConfig); _retryState = RetryStateStorage.LoadRetryState(_storage); @@ -78,7 +80,7 @@ public EventPipeline( internal void UpdateHttpConfig(HttpConfig config) { var retryConfig = config != null - ? new RetryConfig(config.RateLimitConfig, config.BackoffConfig) + ? new RetryConfig(config.RateLimitConfig.Validated(), config.BackoffConfig.Validated()) : new RetryConfig(); _retryStateMachine = new RetryStateMachine(retryConfig); } diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index c17443e..9c283f3 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -86,7 +86,9 @@ public SyncEventPipeline( _flushCancellationToken = flushCancellationToken ?? CancellationToken.None; var retryConfig = httpConfig != null - ? new RetryConfig(httpConfig.RateLimitConfig, httpConfig.BackoffConfig) + // Validated(): user-supplied config reaches us unclamped, unlike the + // CDN path which HttpConfigParser already validates. + ? new RetryConfig(httpConfig.RateLimitConfig.Validated(), httpConfig.BackoffConfig.Validated()) : new RetryConfig(); _retryStateMachine = new RetryStateMachine(retryConfig); _retryState = RetryStateStorage.LoadRetryState(_storage); @@ -95,7 +97,7 @@ public SyncEventPipeline( internal void UpdateHttpConfig(HttpConfig config) { var retryConfig = config != null - ? new RetryConfig(config.RateLimitConfig, config.BackoffConfig) + ? new RetryConfig(config.RateLimitConfig.Validated(), config.BackoffConfig.Validated()) : new RetryConfig(); _retryStateMachine = new RetryStateMachine(retryConfig); } diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 26ff94e..7effcbe 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -85,6 +85,35 @@ public void SyncEventPipeline_WithoutHttpConfig_IsLegacyMode() Assert.True(pipeline._retryStateMachine.IsLegacyMode); } + [Fact] + public void BackoffConfig_DoesNotShareTheDefaultOverrideMap() + { + var first = new BackoffConfig(enabled: true); + first.StatusCodeOverrides[500] = RetryBehavior.Drop; + + var second = new BackoffConfig(enabled: true); + + Assert.False(second.StatusCodeOverrides.ContainsKey(500)); + Assert.NotSame(first.StatusCodeOverrides, second.StatusCodeOverrides); + } + + [Fact] + public void UserSuppliedHttpConfig_IsValidatedOnTheWayIn() + { + // maxRetryInterval: 0 is out of range and must clamp to 1 second, exactly as the + // CDN path does via HttpConfigParser. Unvalidated it would schedule the retry at + // currentTime, i.e. no wait at all. + Analytics analytics = CreateAnalytics( + new HttpConfig(rateLimitConfig: new RateLimitConfig(enabled: true, maxRetryInterval: 0))); + + var pipeline = (EventPipeline)new EventPipelineProvider().Create(analytics, "key"); + RetryState state = pipeline._retryStateMachine.HandleResponse( + new RetryState(), + new ResponseInfo(429, retryAfterSeconds: null, batchFile: "b.json", currentTime: 1000)); + + Assert.Equal(2000, state.WaitUntilTime); + } + [Fact] public void SyncEventPipeline_WithHttpConfig_LeavesLegacyMode() { From 192741b5701359f060efe62e4cd47a3ae259f11a Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 13:14:22 -0400 Subject: [PATCH 03/15] Document that CDN settings replace Configuration.HttpConfig 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. --- Analytics-CSharp/Segment/Analytics/Configuration.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Configuration.cs b/Analytics-CSharp/Segment/Analytics/Configuration.cs index b4ed621..c1f0495 100644 --- a/Analytics-CSharp/Segment/Analytics/Configuration.cs +++ b/Analytics-CSharp/Segment/Analytics/Configuration.cs @@ -49,8 +49,15 @@ private set public IEventPipelineProvider EventPipelineProvider { get; } /// - /// HTTP retry configuration for rate limiting and exponential backoff. - /// Defaults to null, meaning retry settings come from CDN settings alone. + /// HTTP retry configuration for rate limiting and exponential backoff. Defaults to + /// null. + /// + /// This sets the pipeline's starting configuration only. CDN settings take precedence: + /// any settings payload carrying an httpConfig key replaces this value, and a CDN + /// payload is treated as enabling a subsystem unless it says "enabled": "false". + /// A payload with no httpConfig key leaves this value in effect. This matches the + /// behaviour of analytics-kotlin and analytics-swift. + /// /// public HttpConfig HttpConfig { get; } @@ -80,7 +87,7 @@ private set /// defaults to DefaultHTTPClientProvider /// /// set custom flush policies to tell analytics when and how to flush. If a value is given, it overwrites flushAt and flushInterval - /// retry configuration for rate limiting and exponential backoff. CDN settings, when present, take precedence + /// starting retry configuration for rate limiting and exponential backoff. CDN settings, when present, replace it — see public Configuration(string writeKey, int flushAt = 20, int flushInterval = 30, From 8e5dee2a4f8ed73f4a01d5c5febe52ddb7274c0e Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 14:39:23 -0400 Subject: [PATCH 04/15] Expose HttpConfig as a settable property, not a ctor parameter 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. --- Analytics-CSharp/Segment/Analytics/Configuration.cs | 11 +++++------ Tests/Retry/ConfigurationHttpConfigTest.cs | 8 +++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Configuration.cs b/Analytics-CSharp/Segment/Analytics/Configuration.cs index c1f0495..c2aa395 100644 --- a/Analytics-CSharp/Segment/Analytics/Configuration.cs +++ b/Analytics-CSharp/Segment/Analytics/Configuration.cs @@ -50,7 +50,9 @@ private set /// /// HTTP retry configuration for rate limiting and exponential backoff. Defaults to - /// null. + /// null. Set it before constructing Analytics, e.g. + /// new Configuration("writeKey") { HttpConfig = new HttpConfig(...) }. + /// Mirrors analytics-kotlin's mutable Configuration.httpConfig. /// /// This sets the pipeline's starting configuration only. CDN settings take precedence: /// any settings payload carrying an httpConfig key replaces this value, and a CDN @@ -59,7 +61,7 @@ private set /// behaviour of analytics-kotlin and analytics-swift. /// /// - public HttpConfig HttpConfig { get; } + public HttpConfig HttpConfig { get; set; } /// /// Configuration that analytics can use @@ -87,7 +89,6 @@ private set /// defaults to DefaultHTTPClientProvider /// /// set custom flush policies to tell analytics when and how to flush. If a value is given, it overwrites flushAt and flushInterval - /// starting retry configuration for rate limiting and exponential backoff. CDN settings, when present, replace it — see public Configuration(string writeKey, int flushAt = 20, int flushInterval = 30, @@ -100,8 +101,7 @@ public Configuration(string writeKey, IStorageProvider storageProvider = default, IHTTPClientProvider httpClientProvider = default, IList flushPolicies = default, - IEventPipelineProvider eventPipelineProvider = default, - HttpConfig httpConfig = null) + IEventPipelineProvider eventPipelineProvider = default) { WriteKey = writeKey; FlushAt = flushAt; @@ -118,7 +118,6 @@ public Configuration(string writeKey, FlushPolicies.Add(new CountFlushPolicy(flushAt)); FlushPolicies.Add(new FrequencyFlushPolicy(flushInterval * 1000L)); EventPipelineProvider = eventPipelineProvider ?? new EventPipelineProvider(); - HttpConfig = httpConfig; } public Configuration(string writeKey, diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 7effcbe..6e15dc5 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -31,9 +31,11 @@ private static Analytics CreateAnalytics(HttpConfig httpConfig) flushInterval: 0, flushAt: 2, httpClientProvider: new MockHttpClientProvider(mockHttpClient), - storageProvider: new MockStorageProvider(new Mock()), - httpConfig: httpConfig - ); + storageProvider: new MockStorageProvider(new Mock()) + ) + { + HttpConfig = httpConfig + }; return new Analytics(config); } From a294a85efc62f92563a4f68cb094c93450133db8 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 11 Sep 2026 11:11:23 -0400 Subject: [PATCH 05/15] Tighten retry comments 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. --- Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs | 4 ++-- Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs | 4 ++-- .../Segment/Analytics/Utilities/SyncEventPipeline.cs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 3f83581..35dbc42 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -57,8 +57,8 @@ public BackoffConfig( Default4xxBehavior = default4xxBehavior; Default5xxBehavior = default5xxBehavior; UnknownCodeBehavior = unknownCodeBehavior; - // Copy: the property is public, and sharing the static default would let one - // caller's mutation corrupt every BackoffConfig built afterwards in the process. + // Copied because the property is public: sharing the static default would let + // one caller's mutation corrupt every BackoffConfig built afterwards. StatusCodeOverrides = new Dictionary( statusCodeOverrides ?? DefaultStatusCodeOverrides); } diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index 98c14bd..2342d8c 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -69,8 +69,8 @@ public EventPipeline( Running = false; var retryConfig = httpConfig != null - // Validated(): user-supplied config reaches us unclamped, unlike the - // CDN path which HttpConfigParser already validates. + // User-supplied config arrives unclamped; the CDN path is already + // validated by HttpConfigParser. ? new RetryConfig(httpConfig.RateLimitConfig.Validated(), httpConfig.BackoffConfig.Validated()) : new RetryConfig(); _retryStateMachine = new RetryStateMachine(retryConfig); diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index 9c283f3..5678622 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -86,8 +86,8 @@ public SyncEventPipeline( _flushCancellationToken = flushCancellationToken ?? CancellationToken.None; var retryConfig = httpConfig != null - // Validated(): user-supplied config reaches us unclamped, unlike the - // CDN path which HttpConfigParser already validates. + // User-supplied config arrives unclamped; the CDN path is already + // validated by HttpConfigParser. ? new RetryConfig(httpConfig.RateLimitConfig.Validated(), httpConfig.BackoffConfig.Validated()) : new RetryConfig(); _retryStateMachine = new RetryStateMachine(retryConfig); From 983719f215ef4622753cc947f8dbe4b6febbb1f1 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Mon, 21 Sep 2026 15:20:57 -0400 Subject: [PATCH 06/15] Meet the remaining TAPI HTTP agreements: Authorization header, 511, generic Retry-After (529) (#148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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(":"). 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. --- .../Segment/Analytics/Retry/RetryConfig.cs | 6 +- .../Analytics/Retry/RetryStateMachine.cs | 42 ++++++++-- .../Analytics/Utilities/EventPipeline.cs | 8 +- .../Segment/Analytics/Utilities/HTTPClient.cs | 14 +++- .../Analytics/Utilities/RetryAfterParser.cs | 38 +++++++++ .../Analytics/Utilities/SyncEventPipeline.cs | 8 +- Samples/UnitySample/UnityHTTPClient.cs | 1 + Tests/Retry/RetryAfterDeleteBatchTest.cs | 82 +++++++++++++++++++ Tests/Retry/RetryAfterParserTest.cs | 81 ++++++++++++++++++ Tests/Retry/RetryStateMachineTest.cs | 66 +++++++++++++++ Tests/Utilities/HttpClientAuthTest.cs | 42 ++++++++++ e2e-cli/e2e-config.json | 3 +- 12 files changed, 368 insertions(+), 23 deletions(-) create mode 100644 Analytics-CSharp/Segment/Analytics/Utilities/RetryAfterParser.cs create mode 100644 Tests/Retry/RetryAfterDeleteBatchTest.cs create mode 100644 Tests/Retry/RetryAfterParserTest.cs create mode 100644 Tests/Utilities/HttpClientAuthTest.cs diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 35dbc42..fa36226 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -96,7 +96,11 @@ private static Dictionary ValidateOverrides( { 429, RetryBehavior.Retry }, { 460, RetryBehavior.Retry }, { 501, RetryBehavior.Drop }, - { 505, RetryBehavior.Drop } + { 505, RetryBehavior.Drop }, + // 511 is only retryable for an SDK that can re-authenticate via OAuth. + // This one cannot, so retrying would spend the budget on a request that + // can never succeed. + { 511, RetryBehavior.Drop } }; } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 3538bc8..deacdf5 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -22,7 +22,7 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) { if (IsLegacyMode) { - if (response.StatusCode >= 200 && response.StatusCode <= 299) + if (response.StatusCode >= 200 && response.StatusCode < 400) return state.RemoveBatch(response.BatchFile); if (response.StatusCode == 429 || (response.StatusCode >= 500 && response.StatusCode <= 599)) return state; // Keep @@ -31,7 +31,7 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) long currentTime = response.CurrentTime; - if (response.StatusCode >= 200 && response.StatusCode <= 299) + if (response.StatusCode >= 200 && response.StatusCode < 400) { return state.With( pipelineState: PipelineState.Ready, @@ -41,6 +41,16 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) ); } + // Any retryable status with Retry-After → rate-limit path + if (response.RetryAfterSeconds.HasValue && response.RetryAfterSeconds.Value > 0) + { + RetryBehavior behavior = response.StatusCode == 429 + ? RetryBehavior.Retry // 429 is always retryable + : ResolveStatusCodeBehavior(response.StatusCode); + if (behavior == RetryBehavior.Retry && _config.RateLimitConfig.Enabled) + return HandleRateLimitResponse(state, response, currentTime); + } + if (response.StatusCode == 429) { if (_config.RateLimitConfig.Enabled) @@ -48,8 +58,8 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) return state.RemoveBatch(response.BatchFile); } - RetryBehavior behavior = ResolveStatusCodeBehavior(response.StatusCode); - if (behavior == RetryBehavior.Retry && _config.BackoffConfig.Enabled) + RetryBehavior statusBehavior = ResolveStatusCodeBehavior(response.StatusCode); + if (statusBehavior == RetryBehavior.Retry && _config.BackoffConfig.Enabled) return HandleRetryableError(state, response, currentTime); return state.RemoveBatch(response.BatchFile); @@ -131,20 +141,36 @@ public int GetRetryCount(RetryState state, string batchFile) return Math.Max(batchRetryCount, state.GlobalRetryCount); } - public bool ShouldDeleteBatch(int statusCode) + public bool ShouldDeleteBatch(int statusCode) => ShouldDeleteBatch(statusCode, null); + + /// + /// Whether the batch file should be removed. must be + /// the same value handed to , so that the two agree on whether + /// this response took the rate-limit path. + /// + public bool ShouldDeleteBatch(int statusCode, int? retryAfterSeconds) { if (IsLegacyMode) return statusCode >= 400 && statusCode <= 499 && statusCode != 429; - if (statusCode >= 200 && statusCode <= 299) + // Spec item 1: 2xx and 3xx are success. + if (statusCode >= 200 && statusCode < 400) return true; if (statusCode == 429) return !_config.RateLimitConfig.Enabled; RetryBehavior behavior = ResolveStatusCodeBehavior(statusCode); - if (behavior == RetryBehavior.Retry && !_config.BackoffConfig.Enabled) - return true; + if (behavior == RetryBehavior.Retry) + { + // A usable Retry-After sends this response down the rate-limit path, which has + // just scheduled the retry — keep the batch that retry will re-upload. + if (retryAfterSeconds.HasValue && retryAfterSeconds.Value > 0 && _config.RateLimitConfig.Enabled) + return false; + + // Otherwise only backoff can retry it; with backoff off, nothing will. + return !_config.BackoffConfig.Enabled; + } return behavior == RetryBehavior.Drop; } diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index 2342d8c..1eac511 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -211,11 +211,7 @@ await Scope.WithContext(_analytics.FileIODispatcher, () => HTTPClient.Response response = await _httpClient.UploadWithResponse(data, retryCount); statusCode = response.StatusCode; - if (!string.IsNullOrEmpty(response.RetryAfterHeader) - && int.TryParse(response.RetryAfterHeader.Trim(), out int parsedRetryAfter)) - { - retryAfterSeconds = parsedRetryAfter; - } + retryAfterSeconds = RetryAfterParser.Parse(response.RetryAfterHeader); if (response.IsSuccessStatusCode) { @@ -225,7 +221,7 @@ await Scope.WithContext(_analytics.FileIODispatcher, () => else { Analytics.Logger.Log(LogLevel.Error, message: "Error " + statusCode + " uploading " + url); - shouldCleanup = _retryStateMachine.ShouldDeleteBatch(statusCode); + shouldCleanup = _retryStateMachine.ShouldDeleteBatch(statusCode, retryAfterSeconds); if (shouldCleanup) { _analytics.ReportInternalError(AnalyticsErrorType.NetworkServerRejected, diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs index 22b0168..b6946c0 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs @@ -5,6 +5,7 @@ using System.Net; using System.Net.Http; using System.Net.Http.Headers; +using System.Text; using System.Threading.Tasks; using Segment.Analytics.Retry; using Segment.Serialization; @@ -25,6 +26,15 @@ public abstract class HTTPClient private readonly string _apiKey; + /// + /// Value for the Authorization header: the write key as HTTP Basic credentials with an + /// empty password, matching the other Segment SDKs. TAPI authenticates and routes on this + /// header rather than parsing the payload, so custom + /// implementations should send it on upload requests. + /// + protected string BasicAuthorization => + "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes(_apiKey + ":")); + protected readonly string _apiHost; protected readonly string _cdnHost; @@ -188,7 +198,8 @@ public class Response /// /// A convenient method to check if the http request is successful /// - public bool IsSuccessStatusCode => StatusCode >= 200 && StatusCode < 300; + // Spec item 1: 2xx and 3xx are success. + public bool IsSuccessStatusCode => StatusCode >= 200 && StatusCode < 400; } } @@ -242,6 +253,7 @@ public override async Task DoPost(string url, byte[] data, int retryCo var request = new HttpRequestMessage(HttpMethod.Post, url); request.Headers.Add("Connection", "close"); + request.Headers.Add("Authorization", BasicAuthorization); request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/plain")); if (retryCount > 0) request.Headers.Add("X-Retry-Count", retryCount.ToString()); diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/RetryAfterParser.cs b/Analytics-CSharp/Segment/Analytics/Utilities/RetryAfterParser.cs new file mode 100644 index 0000000..7a5f249 --- /dev/null +++ b/Analytics-CSharp/Segment/Analytics/Utilities/RetryAfterParser.cs @@ -0,0 +1,38 @@ +using System; +using System.Globalization; + +namespace Segment.Analytics.Utilities +{ + internal static class RetryAfterParser + { + /// + /// Parses a Retry-After header value. Supports both integer seconds and HTTP-date (RFC 1123) format. + /// Returns the number of seconds to wait, or null if the header is empty/unparseable/in the past. + /// + internal static int? Parse(string headerValue, DateTimeOffset? now = null) + { + if (string.IsNullOrEmpty(headerValue)) + return null; + + string trimmed = headerValue.Trim(); + + if (int.TryParse(trimmed, out int parsedInt)) + { + return parsedInt; + } + + if (DateTimeOffset.TryParseExact(trimmed, + new[] { "r", "ddd, dd MMM yyyy HH:mm:ss 'GMT'" }, + CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal, + out DateTimeOffset targetDate)) + { + DateTimeOffset reference = now ?? DateTimeOffset.UtcNow; + int seconds = (int)(targetDate - reference).TotalSeconds; + return seconds > 0 ? seconds : (int?)null; + } + + return null; + } + } +} diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index 5678622..25ad712 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -236,11 +236,7 @@ await Scope.WithContext(_analytics.FileIODispatcher, () => HTTPClient.Response response = await _httpClient.UploadWithResponse(data, retryCount); statusCode = response.StatusCode; - if (!string.IsNullOrEmpty(response.RetryAfterHeader) - && int.TryParse(response.RetryAfterHeader.Trim(), out int parsedRetryAfter)) - { - retryAfterSeconds = parsedRetryAfter; - } + retryAfterSeconds = RetryAfterParser.Parse(response.RetryAfterHeader); if (response.IsSuccessStatusCode) { @@ -250,7 +246,7 @@ await Scope.WithContext(_analytics.FileIODispatcher, () => else { Analytics.Logger.Log(LogLevel.Error, message: "Error " + statusCode + " uploading " + url); - shouldCleanup = _retryStateMachine.ShouldDeleteBatch(statusCode); + shouldCleanup = _retryStateMachine.ShouldDeleteBatch(statusCode, retryAfterSeconds); if (shouldCleanup) { _analytics.ReportInternalError(AnalyticsErrorType.NetworkServerRejected, diff --git a/Samples/UnitySample/UnityHTTPClient.cs b/Samples/UnitySample/UnityHTTPClient.cs index 60cb8e0..0b87d2d 100644 --- a/Samples/UnitySample/UnityHTTPClient.cs +++ b/Samples/UnitySample/UnityHTTPClient.cs @@ -61,6 +61,7 @@ IEnumerator PostRequest(NetworkRequest networkRequest) using (var request = UnityWebRequest.Put(networkRequest.URL, networkRequest.Data)) { request.SetRequestHeader("Content-Type", "text/plain"); + request.SetRequestHeader("Authorization", BasicAuthorization); yield return request.SendWebRequest(); networkRequest.Response.StatusCode = (int)request.responseCode; diff --git a/Tests/Retry/RetryAfterDeleteBatchTest.cs b/Tests/Retry/RetryAfterDeleteBatchTest.cs new file mode 100644 index 0000000..fa51e16 --- /dev/null +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -0,0 +1,82 @@ +using Segment.Analytics.Retry; +using Xunit; + +namespace Tests.Retry +{ + /// + /// ShouldDeleteBatch must agree with HandleResponse about whether a response took the + /// rate-limit path. A retryable status carrying Retry-After schedules a retry, so its + /// batch must be kept; without Retry-After only backoff can retry it. + /// + public class RetryAfterDeleteBatchTest + { + private static RetryStateMachine RateLimitOnlyMachine() => + new RetryStateMachine(new RetryConfig( + new RateLimitConfig(enabled: true), + new BackoffConfig(enabled: false))); + + [Theory] + [InlineData(503)] + [InlineData(529)] + [InlineData(408)] + [InlineData(410)] + public void RetryableStatus_WithRetryAfter_IsKept(int status) + { + Assert.False(RateLimitOnlyMachine().ShouldDeleteBatch(status, 30)); + } + + [Theory] + [InlineData(503)] + [InlineData(529)] + public void RetryableStatus_WithoutRetryAfter_AndBackoffDisabled_IsDeleted(int status) + { + // Nothing would retry it, so holding the file would leak storage. + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(status, null)); + } + + [Fact] + public void RetryAfterZero_DoesNotCountAsRateLimited() + { + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(503, 0)); + } + + [Fact] + public void RetryAfter_RateLimitsPipelineAndKeepsBatch() + { + var machine = RateLimitOnlyMachine(); + var response = new ResponseInfo(503, retryAfterSeconds: 30, batchFile: "b.json", currentTime: 1000); + + RetryState state = machine.HandleResponse(new RetryState(), response); + + Assert.Equal(PipelineState.RateLimited, state.PipelineState); + Assert.Equal(31000, state.WaitUntilTime); + Assert.False(machine.ShouldDeleteBatch(503, 30)); + } + + [Theory] + [InlineData(200)] + [InlineData(201)] + [InlineData(301)] + [InlineData(304)] + public void SuccessStatuses_AreDeleted(int status) + { + // Spec item 1: 2xx and 3xx are success, so the batch is done with. + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(status, null)); + } + + [Fact] + public void NonRetryableStatus_IsDeletedEvenWithRetryAfter() + { + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(400, 30)); + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(501, 30)); + } + + [Fact] + public void NetworkAuthenticationRequired_IsDropped() + { + // 511 is retryable only for an SDK that can re-authenticate; this one cannot. + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(511, null)); + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(511, 30)); + } + } +} diff --git a/Tests/Retry/RetryAfterParserTest.cs b/Tests/Retry/RetryAfterParserTest.cs new file mode 100644 index 0000000..4880a83 --- /dev/null +++ b/Tests/Retry/RetryAfterParserTest.cs @@ -0,0 +1,81 @@ +using System; +using Segment.Analytics.Utilities; +using Xunit; + +namespace Tests.Retry +{ + public class RetryAfterParserTest + { + [Fact] + public void Parse_IntegerSeconds_ReturnsParsedValue() + { + Assert.Equal(60, RetryAfterParser.Parse("60")); + } + + [Fact] + public void Parse_IntegerWithWhitespace_ReturnsParsedValue() + { + Assert.Equal(120, RetryAfterParser.Parse(" 120 ")); + } + + [Fact] + public void Parse_Null_ReturnsNull() + { + Assert.Null(RetryAfterParser.Parse(null)); + } + + [Fact] + public void Parse_Empty_ReturnsNull() + { + Assert.Null(RetryAfterParser.Parse("")); + } + + [Fact] + public void Parse_HttpDate_InFuture_ReturnsSeconds() + { + var now = new DateTimeOffset(2026, 6, 16, 12, 0, 0, TimeSpan.Zero); + // 2 seconds in the future + string httpDate = "Tue, 16 Jun 2026 12:00:02 GMT"; + + int? result = RetryAfterParser.Parse(httpDate, now); + + Assert.Equal(2, result); + } + + [Fact] + public void Parse_HttpDate_InPast_ReturnsNull() + { + var now = new DateTimeOffset(2026, 6, 16, 12, 0, 0, TimeSpan.Zero); + // 10 seconds in the past + string httpDate = "Tue, 16 Jun 2026 11:59:50 GMT"; + + int? result = RetryAfterParser.Parse(httpDate, now); + + Assert.Null(result); + } + + [Fact] + public void Parse_HttpDate_Rfc1123Format_ParsesCorrectly() + { + var now = new DateTimeOffset(2026, 6, 16, 10, 0, 0, TimeSpan.Zero); + // 300 seconds (5 minutes) in the future + string httpDate = "Tue, 16 Jun 2026 10:05:00 GMT"; + + int? result = RetryAfterParser.Parse(httpDate, now); + + Assert.Equal(300, result); + } + + [Fact] + public void Parse_InvalidString_ReturnsNull() + { + Assert.Null(RetryAfterParser.Parse("not-a-date-or-number")); + } + + [Fact] + public void Parse_Zero_ReturnsZero() + { + Assert.Equal(0, RetryAfterParser.Parse("0")); + } + } +} diff --git a/Tests/Retry/RetryStateMachineTest.cs b/Tests/Retry/RetryStateMachineTest.cs index 69b0250..1e27178 100644 --- a/Tests/Retry/RetryStateMachineTest.cs +++ b/Tests/Retry/RetryStateMachineTest.cs @@ -371,6 +371,72 @@ public void ShouldDeleteBatch_SmartMode_408_False() Assert.False(machine.ShouldDeleteBatch(408)); } + // --- RetryAfterSeconds on retryable errors --- + + [Fact] + public void HandleResponse_503_WithRetryAfter_RoutesToRateLimitPath() + { + var machine = CreateMachine(maxRetryInterval: 300); + var state = new RetryState(); + var response = new ResponseInfo(503, retryAfterSeconds: 2, batchFile: "batch1.json", currentTime: 1000); + + RetryState newState = machine.HandleResponse(state, response); + + Assert.Equal(PipelineState.RateLimited, newState.PipelineState); + Assert.Equal(1, newState.GlobalRetryCount); + Assert.Equal(1000L + 2000L, newState.WaitUntilTime); + } + + [Fact] + public void HandleResponse_529_WithRetryAfter_RoutesToRateLimitPath() + { + var machine = CreateMachine(maxRetryInterval: 300); + var state = new RetryState(); + var response = new ResponseInfo(529, retryAfterSeconds: 3, batchFile: "batch1.json", currentTime: 1000); + + RetryState newState = machine.HandleResponse(state, response); + + Assert.Equal(PipelineState.RateLimited, newState.PipelineState); + Assert.Equal(1, newState.GlobalRetryCount); + Assert.Equal(1000L + 3000L, newState.WaitUntilTime); + } + + [Fact] + public void HandleResponse_503_WithoutRetryAfter_UsesExponentialBackoff() + { + var machine = CreateMachine(); + var state = new RetryState(); + var response = new ResponseInfo(503, retryAfterSeconds: null, batchFile: "batch1.json", currentTime: 1000); + + RetryState newState = machine.HandleResponse(state, response); + + // Still goes through backoff path (failureCount incremented, not rate-limited) + Assert.True(newState.BatchMetadata.ContainsKey("batch1.json")); + Assert.Equal(1, newState.BatchMetadata["batch1.json"].FailureCount); + Assert.True(newState.BatchMetadata["batch1.json"].NextRetryTime > 1000L); + Assert.Equal(PipelineState.Ready, newState.PipelineState); + Assert.Equal(0, newState.GlobalRetryCount); + } + + [Fact] + public void HandleResponse_503_WithRetryAfter_ClampsToMaxRetryInterval() + { + var config = new RetryConfig( + new RateLimitConfig(enabled: true, maxRetryCount: 100, maxRetryInterval: 10), + new BackoffConfig(enabled: true, maxRetryCount: 100, maxBackoffInterval: 300) + ); + var machine = new RetryStateMachine(config, new FakeTimeProvider(), new Random(42)); + var state = new RetryState(); + var response = new ResponseInfo(503, retryAfterSeconds: 999, batchFile: "batch1.json", currentTime: 1000); + + RetryState newState = machine.HandleResponse(state, response); + + // Now routes through rate-limit path, clamped to maxRetryInterval=10 + Assert.Equal(PipelineState.RateLimited, newState.PipelineState); + Assert.Equal(1000L + 10 * 1000L, newState.WaitUntilTime); + Assert.Equal(1, newState.GlobalRetryCount); + } + // --- GetRetryCount tests --- [Fact] diff --git a/Tests/Utilities/HttpClientAuthTest.cs b/Tests/Utilities/HttpClientAuthTest.cs new file mode 100644 index 0000000..d4fd9b8 --- /dev/null +++ b/Tests/Utilities/HttpClientAuthTest.cs @@ -0,0 +1,42 @@ +using System; +using System.Text; +using Segment.Analytics.Utilities; +using Xunit; + +namespace Tests.Utilities +{ + /// + /// TAPI authenticates and routes on the Authorization header rather than parsing the + /// payload, so the value has to match what the other Segment SDKs send: the write key + /// as Basic credentials with an empty password. + /// + public class HttpClientAuthTest + { + private class AuthProbe : DefaultHTTPClient + { + public AuthProbe(string apiKey) : base(apiKey) { } + + public string Authorization => BasicAuthorization; + } + + [Theory] + [InlineData("writekey123")] + [InlineData("aBc-123_XYZ")] + public void UsesWriteKeyAsBasicCredentialsWithEmptyPassword(string writeKey) + { + var expected = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes(writeKey + ":")); + + Assert.Equal(expected, new AuthProbe(writeKey).Authorization); + } + + [Fact] + public void EncodesTheTrailingColonSeparator() + { + // Decoding must yield ":" — an empty password, not a missing one. + var header = new AuthProbe("k").Authorization; + var decoded = Encoding.UTF8.GetString(Convert.FromBase64String(header.Substring("Basic ".Length))); + + Assert.Equal("k:", decoded); + } + } +} diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index 91e0a5f..7237fc7 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -4,6 +4,7 @@ "auto_settings": true, "patch": null, "env": { - "HTTP_CONFIG_SETTINGS": "true" + "HTTP_CONFIG_SETTINGS": "true", + "AUTH_HEADER": "true" } } From 7593a634748f732e26b6689bca68cbdc48dd4476 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 12:04:20 -0400 Subject: [PATCH 07/15] 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. HttpClient follows what it can. Three sites narrowed, and the tests now assert 300/301/304 are not success rather than that they are. --- .../Analytics/Retry/RetryStateMachine.cs | 7 +++--- .../Segment/Analytics/Utilities/HTTPClient.cs | 5 +++-- Tests/Retry/RetryAfterDeleteBatchTest.cs | 22 ++++++++++++++++--- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index deacdf5..853d9ed 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -22,7 +22,7 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) { if (IsLegacyMode) { - if (response.StatusCode >= 200 && response.StatusCode < 400) + if (response.StatusCode >= 200 && response.StatusCode <= 299) return state.RemoveBatch(response.BatchFile); if (response.StatusCode == 429 || (response.StatusCode >= 500 && response.StatusCode <= 599)) return state; // Keep @@ -31,7 +31,7 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) long currentTime = response.CurrentTime; - if (response.StatusCode >= 200 && response.StatusCode < 400) + if (response.StatusCode >= 200 && response.StatusCode <= 299) { return state.With( pipelineState: PipelineState.Ready, @@ -153,8 +153,7 @@ public bool ShouldDeleteBatch(int statusCode, int? retryAfterSeconds) if (IsLegacyMode) return statusCode >= 400 && statusCode <= 499 && statusCode != 429; - // Spec item 1: 2xx and 3xx are success. - if (statusCode >= 200 && statusCode < 400) + if (statusCode >= 200 && statusCode <= 299) return true; if (statusCode == 429) diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs index b6946c0..fd5cd0e 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs @@ -198,8 +198,9 @@ public class Response /// /// A convenient method to check if the http request is successful /// - // Spec item 1: 2xx and 3xx are success. - public bool IsSuccessStatusCode => StatusCode >= 200 && StatusCode < 400; + // Only 2xx. HttpClient follows any redirect it can, so a 3xx here means it + // declined to (no Location, a 300, or a 304) and nothing was uploaded. + public bool IsSuccessStatusCode => StatusCode >= 200 && StatusCode < 300; } } diff --git a/Tests/Retry/RetryAfterDeleteBatchTest.cs b/Tests/Retry/RetryAfterDeleteBatchTest.cs index fa51e16..8a3d12a 100644 --- a/Tests/Retry/RetryAfterDeleteBatchTest.cs +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -56,14 +56,30 @@ public void RetryAfter_RateLimitsPipelineAndKeepsBatch() [Theory] [InlineData(200)] [InlineData(201)] - [InlineData(301)] - [InlineData(304)] + [InlineData(204)] public void SuccessStatuses_AreDeleted(int status) { - // Spec item 1: 2xx and 3xx are success, so the batch is done with. + // 2xx is success, so the batch is done with. Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(status, null)); } + [Theory] + [InlineData(300)] + [InlineData(301)] + [InlineData(304)] + public void Redirects_AreNotSuccess(int status) + { + // HttpClient follows what it can; a 3xx arriving here means nothing was + // uploaded, so the batch must not be treated as delivered. + var machine = RateLimitOnlyMachine(); + RetryState state = machine.HandleResponse( + new RetryState(), + new ResponseInfo(status, retryAfterSeconds: null, batchFile: "b.json", currentTime: 1000)); + + Assert.Equal(PipelineState.Ready, state.PipelineState); + Assert.True(machine.ShouldDeleteBatch(status, null)); + } + [Fact] public void NonRetryableStatus_IsDeletedEvenWithRetryAfter() { From caff264b12883ba89482701cccf12e5cad8541e4 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:47:14 -0400 Subject: [PATCH 08/15] Floor maxRetryCount at 1 so zero does not drop everything unsent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Segment/Analytics/Retry/RetryConfig.cs | 6 ++++-- Tests/Retry/ConfigurationHttpConfigTest.cs | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index fa36226..e371e75 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -18,7 +18,9 @@ public RateLimitConfig(bool enabled = false, int maxRetryCount = 100, int maxRet public RateLimitConfig Validated() => new RateLimitConfig( enabled: Enabled, - maxRetryCount: Math.Max(0, Math.Min(MaxRetryCount, 1000)), + // Floored at 1: the count is compared against a fresh state's retry + // count, so 0 would drop every batch before it was ever sent. + maxRetryCount: Math.Max(1, Math.Min(MaxRetryCount, 1000)), maxRetryInterval: Math.Max(1, Math.Min(MaxRetryInterval, 3600)) ); } @@ -65,7 +67,7 @@ public BackoffConfig( public BackoffConfig Validated() => new BackoffConfig( enabled: Enabled, - maxRetryCount: Math.Max(0, Math.Min(MaxRetryCount, 1000)), + maxRetryCount: Math.Max(1, Math.Min(MaxRetryCount, 1000)), baseBackoffInterval: Math.Max(0.1, Math.Min(BaseBackoffInterval, 60.0)), maxBackoffInterval: Math.Max(1, Math.Min(MaxBackoffInterval, 3600)), maxTotalBackoffDuration: Math.Max(0, Math.Min(MaxTotalBackoffDuration, 604800)), diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 6e15dc5..55add83 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -1,3 +1,4 @@ +using System; using Moq; using Segment.Analytics; using Segment.Analytics.Retry; @@ -87,6 +88,21 @@ public void SyncEventPipeline_WithoutHttpConfig_IsLegacyMode() Assert.True(pipeline._retryStateMachine.IsLegacyMode); } + [Fact] + public void MaxRetryCountOfZero_DoesNotDropBeforeTheFirstAttempt() + { + // ShouldUploadBatch compares a fresh state's counts against MaxRetryCount, + // so an unfloored 0 dropped every batch without ever sending it. + var machine = new RetryStateMachine(new RetryConfig( + new RateLimitConfig(enabled: true, maxRetryCount: 0).Validated(), + new BackoffConfig(enabled: true, maxRetryCount: 0).Validated())); + + Tuple decision = + machine.ShouldUploadBatch(new RetryState(), "b.json"); + + Assert.IsType(decision.Item1); + } + [Fact] public void BackoffConfig_DoesNotShareTheDefaultOverrideMap() { From 385b47e48f562fcd124fd76d123c0f13f9c3ce02 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:57:14 -0400 Subject: [PATCH 09/15] 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. --- CHANGELOG.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..06c52cd --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,25 @@ +# Changelog + +Release notes for published versions are generated on the +[Releases page](https://github.com/segmentio/Analytics-CSharp/releases). +This file carries the notes that need more than a pull-request title. + +## Unreleased + +### Upgrade note: new request headers and proxy allowlists + +This release sends two request headers that 2.6.0 did not: `Authorization` +(HTTP Basic, carrying your write key) and `X-Retry-Count` (on retries only). +If your traffic to Segment goes through a proxy, gateway or WAF that +allowlists request headers, add both before upgrading or uploads will be +rejected. Unity WebGL builds must also add them to the CORS +`Access-Control-Allow-Headers` allowlist on any proxy they point at. + +- Send the write key as an `Authorization: Basic` header. It is still included in the request body, so no server-side change is required. +- Send `X-Retry-Count` on retries, so the server can distinguish a retry from a first attempt. +- `HttpConfig` is now a settable property on `Configuration` rather than a constructor parameter, so retry behaviour can be configured after construction. For mobile targets, CDN settings replace `Configuration.HttpConfig` when they are present. +- `Retry-After` is honoured on every retryable status rather than 429 alone, which brings 529 in through the generic 5xx rule. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, capped at `MaxRetryInterval`. +- 511 is dropped rather than retried: it asks the client to authenticate, which this library cannot do. +- Only 2xx responses count as a successful upload. A 3xx is now retried rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom host values. +- `RateLimitConfig.MaxRetryCount` and `BackoffConfig.MaxRetryCount` are floored at 1. A configured 0 previously dropped every batch before it was ever sent. +- `BackoffConfig.StatusCodeOverrides` is copied rather than held by reference, so mutating the caller's dictionary no longer changes a live config. From c834c0ef6bbd25c1d57c158892e3f21420daae93 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 07:06:23 -0400 Subject: [PATCH 10/15] 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. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06c52cd..e7c15f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,6 @@ rejected. Unity WebGL builds must also add them to the CORS - `HttpConfig` is now a settable property on `Configuration` rather than a constructor parameter, so retry behaviour can be configured after construction. For mobile targets, CDN settings replace `Configuration.HttpConfig` when they are present. - `Retry-After` is honoured on every retryable status rather than 429 alone, which brings 529 in through the generic 5xx rule. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, capped at `MaxRetryInterval`. - 511 is dropped rather than retried: it asks the client to authenticate, which this library cannot do. -- Only 2xx responses count as a successful upload. A 3xx is now retried rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom host values. +- 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 the HTTP client already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom host values. - `RateLimitConfig.MaxRetryCount` and `BackoffConfig.MaxRetryCount` are floored at 1. A configured 0 previously dropped every batch before it was ever sent. - `BackoffConfig.StatusCodeOverrides` is copied rather than held by reference, so mutating the caller's dictionary no longer changes a live config. From 67d85aa2b9b5190cbfeea5ff3ef25a5d5048b89a Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 08:52:17 -0400 Subject: [PATCH 11/15] Enable retries by default and align the backoff shape 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. --- .../Segment/Analytics/Configuration.cs | 13 ++++--- .../Analytics/Retry/HttpConfigParser.cs | 18 ++++++---- .../Segment/Analytics/Retry/RetryConfig.cs | 8 ++--- .../Analytics/Retry/RetryStateMachine.cs | 5 +++ .../Analytics/Utilities/EventPipeline.cs | 2 +- .../Segment/Analytics/Utilities/HTTPClient.cs | 7 +++- .../Analytics/Utilities/SyncEventPipeline.cs | 2 +- CHANGELOG.md | 26 ++++++++++++++ Tests/Retry/ConfigurationHttpConfigTest.cs | 35 +++++++++++++++++-- Tests/Retry/HttpConfigParserTest.cs | 17 ++++++++- Tests/Retry/RetryStateMachineTest.cs | 7 +++- 11 files changed, 116 insertions(+), 24 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Configuration.cs b/Analytics-CSharp/Segment/Analytics/Configuration.cs index c2aa395..463bd83 100644 --- a/Analytics-CSharp/Segment/Analytics/Configuration.cs +++ b/Analytics-CSharp/Segment/Analytics/Configuration.cs @@ -50,15 +50,18 @@ private set /// /// HTTP retry configuration for rate limiting and exponential backoff. Defaults to - /// null. Set it before constructing Analytics, e.g. + /// null, which runs rate limiting and backoff with their built-in defaults. Pass a + /// config to change them, or one with both subsystems disabled to opt out of retrying. + /// Set it before constructing Analytics, e.g. /// new Configuration("writeKey") { HttpConfig = new HttpConfig(...) }. /// Mirrors analytics-kotlin's mutable Configuration.httpConfig. /// /// This sets the pipeline's starting configuration only. CDN settings take precedence: - /// any settings payload carrying an httpConfig key replaces this value, and a CDN - /// payload is treated as enabling a subsystem unless it says "enabled": "false". - /// A payload with no httpConfig key leaves this value in effect. This matches the - /// behaviour of analytics-kotlin and analytics-swift. + /// any settings payload carrying an httpConfig key replaces the configuration the + /// pipeline is running with — this property keeps the value you set — and a CDN payload is + /// treated as enabling a subsystem unless it says "enabled": "false". A payload with + /// no httpConfig key leaves this value in effect. This matches the behaviour of + /// analytics-kotlin and analytics-swift. /// /// public HttpConfig HttpConfig { get; set; } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs b/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs index 8412944..ddcc789 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs @@ -46,12 +46,14 @@ private static RateLimitConfig ParseRateLimitConfig(JsonObject json, bool enable if (json == null) return new RateLimitConfig(enabled: enabled); - int maxRetryCount = 100; + var defaults = new RateLimitConfig(); + + int maxRetryCount = defaults.MaxRetryCount; string maxRetriesStr = json.GetString("maxRetryCount"); if (maxRetriesStr != null && int.TryParse(maxRetriesStr, out int parsedMaxRetries)) maxRetryCount = parsedMaxRetries; - int maxRetryInterval = 300; + int maxRetryInterval = defaults.MaxRetryInterval; string intervalStr = json.GetString("maxRetryInterval"); if (intervalStr != null && int.TryParse(intervalStr, out int parsedInterval)) maxRetryInterval = parsedInterval; @@ -68,27 +70,29 @@ private static BackoffConfig ParseBackoffConfig(JsonObject json, bool enabled) if (json == null) return new BackoffConfig(enabled: enabled); - int maxRetryCount = 100; + var defaults = new BackoffConfig(); + + int maxRetryCount = defaults.MaxRetryCount; string maxRetriesStr = json.GetString("maxRetryCount"); if (maxRetriesStr != null && int.TryParse(maxRetriesStr, out int parsedMaxRetries)) maxRetryCount = parsedMaxRetries; - double baseBackoffInterval = 0.5; + double baseBackoffInterval = defaults.BaseBackoffInterval; string baseStr = json.GetString("baseBackoffInterval"); if (baseStr != null && double.TryParse(baseStr, NumberStyles.Float, CultureInfo.InvariantCulture, out double parsedBase)) baseBackoffInterval = parsedBase; - int maxBackoffInterval = 300; + int maxBackoffInterval = defaults.MaxBackoffInterval; string maxStr = json.GetString("maxBackoffInterval"); if (maxStr != null && int.TryParse(maxStr, out int parsedMax)) maxBackoffInterval = parsedMax; - long maxTotalBackoffDuration = 43200; + long maxTotalBackoffDuration = defaults.MaxTotalBackoffDuration; string durationStr = json.GetString("maxTotalBackoffDuration"); if (durationStr != null && long.TryParse(durationStr, out long parsedDuration)) maxTotalBackoffDuration = parsedDuration; - int jitterPercent = 10; + int jitterPercent = defaults.JitterPercent; string jitterStr = json.GetString("jitterPercent"); if (jitterStr != null && int.TryParse(jitterStr, out int parsedJitter)) jitterPercent = parsedJitter; diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index e371e75..b4d3857 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -9,7 +9,7 @@ public class RateLimitConfig public int MaxRetryCount { get; } public int MaxRetryInterval { get; } - public RateLimitConfig(bool enabled = false, int maxRetryCount = 100, int maxRetryInterval = 300) + public RateLimitConfig(bool enabled = true, int maxRetryCount = 100, int maxRetryInterval = 300) { Enabled = enabled; MaxRetryCount = maxRetryCount; @@ -39,10 +39,10 @@ public class BackoffConfig public Dictionary StatusCodeOverrides { get; } public BackoffConfig( - bool enabled = false, - int maxRetryCount = 100, + bool enabled = true, + int maxRetryCount = 10, double baseBackoffInterval = 0.5, - int maxBackoffInterval = 300, + int maxBackoffInterval = 60, long maxTotalBackoffDuration = 43200, int jitterPercent = 10, RetryBehavior default4xxBehavior = RetryBehavior.Drop, diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 853d9ed..7f77de7 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -55,6 +55,9 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) { if (_config.RateLimitConfig.Enabled) return HandleRateLimitResponse(state, response, currentTime); + // Dropped rather than handed to backoff: rateLimitConfig.enabled:false is a + // kill switch for 429 handling, symmetric with backoffConfig.enabled:false + // for 5xx. Asserted by the shared e2e suite's settings-enabled-flag tests. return state.RemoveBatch(response.BatchFile); } @@ -156,6 +159,8 @@ public bool ShouldDeleteBatch(int statusCode, int? retryAfterSeconds) if (statusCode >= 200 && statusCode <= 299) return true; + // Matches HandleResponse: with rate limiting off, a 429 is dropped rather than + // falling through to backoff. if (statusCode == 429) return !_config.RateLimitConfig.Enabled; diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index 1eac511..4c754f9 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -221,7 +221,7 @@ await Scope.WithContext(_analytics.FileIODispatcher, () => else { Analytics.Logger.Log(LogLevel.Error, message: "Error " + statusCode + " uploading " + url); - shouldCleanup = _retryStateMachine.ShouldDeleteBatch(statusCode, retryAfterSeconds); + shouldCleanup = retryStateMachine.ShouldDeleteBatch(statusCode, retryAfterSeconds); if (shouldCleanup) { _analytics.ReportInternalError(AnalyticsErrorType.NetworkServerRejected, diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs index fd5cd0e..cfc6b14 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs @@ -133,7 +133,12 @@ public virtual async Task Upload(byte[] data) AnalyticsRef?.ReportInternalError(AnalyticsErrorType.NetworkUnexpectedHttpCode, message: "Response code: " + response.StatusCode); // Single source of truth for the drop/keep decision. - return new RetryStateMachine(new RetryConfig()).ShouldDeleteBatch(response.StatusCode); + // Pinned to a disabled config so this legacy path keeps the drop/keep + // behaviour it had before retries became enabled by default. + return new RetryStateMachine(new RetryConfig( + new RateLimitConfig(enabled: false), + new BackoffConfig(enabled: false))) + .ShouldDeleteBatch(response.StatusCode); } catch (Exception e) { diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index 25ad712..a5328e0 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -246,7 +246,7 @@ await Scope.WithContext(_analytics.FileIODispatcher, () => else { Analytics.Logger.Log(LogLevel.Error, message: "Error " + statusCode + " uploading " + url); - shouldCleanup = _retryStateMachine.ShouldDeleteBatch(statusCode, retryAfterSeconds); + shouldCleanup = retryStateMachine.ShouldDeleteBatch(statusCode, retryAfterSeconds); if (shouldCleanup) { _analytics.ReportInternalError(AnalyticsErrorType.NetworkServerRejected, diff --git a/CHANGELOG.md b/CHANGELOG.md index e7c15f0..b8b2cf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,32 @@ This file carries the notes that need more than a pull-request title. ## Unreleased +### Behaviour change: retries and backoff are on by default + +Through 2.6.0, rate limiting and exponential backoff were both disabled unless you +supplied an `HttpConfig` or a CDN settings payload turned them on. Server-side +deployments receive no CDN settings, so in practice they retried nothing: 408, 410 and +460 were dropped, `Retry-After` was ignored, and a 429 or 5xx was held with no delay and +no budget. Both subsystems now default to enabled, so a client that configures nothing +gets the documented retry behaviour. + +To keep the old behaviour, disable both explicitly: + +```csharp +new Configuration("writeKey") +{ + HttpConfig = new HttpConfig( + new RateLimitConfig(enabled: false), + new BackoffConfig(enabled: false)) +} +``` + +CDN settings are unaffected and still take precedence: a payload carrying an +`httpConfig` key replaces whatever the pipeline is running with, and a payload without +that key leaves your configuration in effect. + +- Backoff defaults now match the other Segment SDKs: `MaxRetryCount` 10 (was 100) and `MaxBackoffInterval` 60s (was 300s). With retries off by default those numbers were latent; enabling them unchanged would have had C# clients making an order of magnitude more attempts against the endpoint than any other SDK. + ### Upgrade note: new request headers and proxy allowlists This release sends two request headers that 2.6.0 did not: `Authorization` diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 55add83..170c169 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -58,12 +58,27 @@ public void Configuration_HttpConfigDefaultsToNull() } [Fact] - public void EventPipeline_WithoutHttpConfig_IsLegacyMode() + public void EventPipeline_WithoutHttpConfig_RetriesByDefault() { + // Server-side users get no CDN settings, so a null HttpConfig has to mean + // "retry with the defaults", not "no retry behaviour at all". Analytics analytics = CreateAnalytics(null); var pipeline = (EventPipeline)new EventPipelineProvider().Create(analytics, "key"); + Assert.False(pipeline._retryStateMachine.IsLegacyMode); + } + + [Fact] + public void EventPipeline_WithBothSubsystemsDisabled_IsLegacyMode() + { + // Opting out is now explicit rather than the default. + Analytics analytics = CreateAnalytics(new HttpConfig( + new RateLimitConfig(enabled: false), + new BackoffConfig(enabled: false))); + + var pipeline = (EventPipeline)new EventPipelineProvider().Create(analytics, "key"); + Assert.True(pipeline._retryStateMachine.IsLegacyMode); } @@ -79,13 +94,27 @@ public void EventPipeline_WithHttpConfig_LeavesLegacyMode() } [Fact] - public void SyncEventPipeline_WithoutHttpConfig_IsLegacyMode() + public void SyncEventPipeline_WithoutHttpConfig_RetriesByDefault() { Analytics analytics = CreateAnalytics(null); var pipeline = (SyncEventPipeline)new SyncEventPipelineProvider().Create(analytics, "key"); - Assert.True(pipeline._retryStateMachine.IsLegacyMode); + Assert.False(pipeline._retryStateMachine.IsLegacyMode); + } + + [Fact] + public void DefaultBackoffShapeMatchesTheOtherSdks() + { + // java, go, python, ruby and php all default to 10 retries and a 60s ceiling. + // These were 100 and 300 while retries were off by default; now that they are + // on, a drift here changes the load every C# client puts on the endpoint. + var backoff = new BackoffConfig(); + + Assert.Equal(10, backoff.MaxRetryCount); + Assert.Equal(60, backoff.MaxBackoffInterval); + Assert.Equal(0.5, backoff.BaseBackoffInterval); + Assert.Equal(43200, backoff.MaxTotalBackoffDuration); } [Fact] diff --git a/Tests/Retry/HttpConfigParserTest.cs b/Tests/Retry/HttpConfigParserTest.cs index 89ff4cf..19fff0c 100644 --- a/Tests/Retry/HttpConfigParserTest.cs +++ b/Tests/Retry/HttpConfigParserTest.cs @@ -101,7 +101,22 @@ public void Parse_PartialConfig_UsesDefaults() Assert.Equal(50, config.BackoffConfig.MaxRetryCount); Assert.Equal(0.5, config.BackoffConfig.BaseBackoffInterval); // default - Assert.Equal(300, config.BackoffConfig.MaxBackoffInterval); // default + Assert.Equal(60, config.BackoffConfig.MaxBackoffInterval); // default + } + + [Fact] + public void Parse_AbsentKeys_MatchTheConstructorDefaults() + { + // The parser used to hardcode its own copies of these, which had drifted. + var json = JsonUtility.FromJson("{\"backoffConfig\":{}}"); + HttpConfig config = HttpConfigParser.Parse(json); + var defaults = new BackoffConfig(); + + Assert.Equal(defaults.MaxRetryCount, config.BackoffConfig.MaxRetryCount); + Assert.Equal(defaults.BaseBackoffInterval, config.BackoffConfig.BaseBackoffInterval); + Assert.Equal(defaults.MaxBackoffInterval, config.BackoffConfig.MaxBackoffInterval); + Assert.Equal(defaults.MaxTotalBackoffDuration, config.BackoffConfig.MaxTotalBackoffDuration); + Assert.Equal(defaults.JitterPercent, config.BackoffConfig.JitterPercent); } } } diff --git a/Tests/Retry/RetryStateMachineTest.cs b/Tests/Retry/RetryStateMachineTest.cs index 1e27178..8ea18df 100644 --- a/Tests/Retry/RetryStateMachineTest.cs +++ b/Tests/Retry/RetryStateMachineTest.cs @@ -97,8 +97,12 @@ public void HandleResponse_429_NullRetryAfter_UsesMaxInterval() } [Fact] - public void HandleResponse_429_RateLimitDisabled_DropsBatch() + public void HandleResponse_429_RateLimitDisabled_DropsBatchRatherThanUsingBackoff() { + // rateLimitConfig.enabled:false is a deliberate kill switch for 429 handling, + // symmetric with backoffConfig.enabled:false for 5xx, and asserted by the shared + // e2e suite (retry-settings/settings-enabled-flag). Do not "fix" this to fall + // through to backoff: that breaks the cross-SDK contract. var machine = CreateMachine(rateLimitEnabled: false, backoffEnabled: true); var state = new RetryState( batchMetadata: new System.Collections.Generic.Dictionary @@ -110,6 +114,7 @@ public void HandleResponse_429_RateLimitDisabled_DropsBatch() RetryState newState = machine.HandleResponse(state, response); Assert.False(newState.BatchMetadata.ContainsKey("batch1.json")); + Assert.True(machine.ShouldDeleteBatch(429, 60)); } [Fact] From 719a9ed83f2ff4312d2fe2c36080ed2df41f4cd5 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 10:04:40 -0400 Subject: [PATCH 12/15] Use the repo's US spelling in the comments I added This repo writes "behavior" 75 times to "behaviour" twice, and both outliers were mine. --- Analytics-CSharp/Segment/Analytics/Configuration.cs | 2 +- .../Segment/Analytics/Utilities/HTTPClient.cs | 2 +- CHANGELOG.md | 8 ++++---- Tests/Retry/ConfigurationHttpConfigTest.cs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Configuration.cs b/Analytics-CSharp/Segment/Analytics/Configuration.cs index 463bd83..c5fba22 100644 --- a/Analytics-CSharp/Segment/Analytics/Configuration.cs +++ b/Analytics-CSharp/Segment/Analytics/Configuration.cs @@ -60,7 +60,7 @@ private set /// any settings payload carrying an httpConfig key replaces the configuration the /// pipeline is running with — this property keeps the value you set — and a CDN payload is /// treated as enabling a subsystem unless it says "enabled": "false". A payload with - /// no httpConfig key leaves this value in effect. This matches the behaviour of + /// no httpConfig key leaves this value in effect. This matches the behavior of /// analytics-kotlin and analytics-swift. /// /// diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs index cfc6b14..d12dedc 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs @@ -134,7 +134,7 @@ public virtual async Task Upload(byte[] data) // Single source of truth for the drop/keep decision. // Pinned to a disabled config so this legacy path keeps the drop/keep - // behaviour it had before retries became enabled by default. + // behavior it had before retries became enabled by default. return new RetryStateMachine(new RetryConfig( new RateLimitConfig(enabled: false), new BackoffConfig(enabled: false))) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8b2cf9..85249bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,16 +6,16 @@ This file carries the notes that need more than a pull-request title. ## Unreleased -### Behaviour change: retries and backoff are on by default +### Behavior change: retries and backoff are on by default Through 2.6.0, rate limiting and exponential backoff were both disabled unless you supplied an `HttpConfig` or a CDN settings payload turned them on. Server-side deployments receive no CDN settings, so in practice they retried nothing: 408, 410 and 460 were dropped, `Retry-After` was ignored, and a 429 or 5xx was held with no delay and no budget. Both subsystems now default to enabled, so a client that configures nothing -gets the documented retry behaviour. +gets the documented retry behavior. -To keep the old behaviour, disable both explicitly: +To keep the old behavior, disable both explicitly: ```csharp new Configuration("writeKey") @@ -43,7 +43,7 @@ rejected. Unity WebGL builds must also add them to the CORS - Send the write key as an `Authorization: Basic` header. It is still included in the request body, so no server-side change is required. - Send `X-Retry-Count` on retries, so the server can distinguish a retry from a first attempt. -- `HttpConfig` is now a settable property on `Configuration` rather than a constructor parameter, so retry behaviour can be configured after construction. For mobile targets, CDN settings replace `Configuration.HttpConfig` when they are present. +- `HttpConfig` is now a settable property on `Configuration` rather than a constructor parameter, so retry behavior can be configured after construction. For mobile targets, CDN settings replace `Configuration.HttpConfig` when they are present. - `Retry-After` is honoured on every retryable status rather than 429 alone, which brings 529 in through the generic 5xx rule. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, capped at `MaxRetryInterval`. - 511 is dropped rather than retried: it asks the client to authenticate, which this library cannot do. - 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 the HTTP client 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/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 170c169..7156dcc 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -61,7 +61,7 @@ public void Configuration_HttpConfigDefaultsToNull() public void EventPipeline_WithoutHttpConfig_RetriesByDefault() { // Server-side users get no CDN settings, so a null HttpConfig has to mean - // "retry with the defaults", not "no retry behaviour at all". + // "retry with the defaults", not "no retry behavior at all". Analytics analytics = CreateAnalytics(null); var pipeline = (EventPipeline)new EventPipelineProvider().Create(analytics, "key"); From 5c6b57e0c990d430e5284829999e2d4cd50350ec Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 11:08:21 -0400 Subject: [PATCH 13/15] Bound the rate-limit path by duration, cap Retry-After at 300s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Analytics/Retry/HttpConfigParser.cs | 8 ++- .../Segment/Analytics/Retry/RetryConfig.cs | 23 ++++++- .../Segment/Analytics/Retry/RetryState.cs | 18 ++++- .../Analytics/Retry/RetryStateMachine.cs | 26 +++++++- .../Analytics/Retry/RetryStateStorage.cs | 5 +- .../Analytics/Utilities/EventPipeline.cs | 2 +- .../Analytics/Utilities/SyncEventPipeline.cs | 2 +- CHANGELOG.md | 2 + Tests/Retry/ConfigurationHttpConfigTest.cs | 27 ++++++++ Tests/Retry/HttpConfigParserTest.cs | 3 +- Tests/Retry/RetryStateMachineTest.cs | 66 ++++++++++++++++++- 11 files changed, 167 insertions(+), 15 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs b/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs index ddcc789..43e8807 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs @@ -58,10 +58,16 @@ private static RateLimitConfig ParseRateLimitConfig(JsonObject json, bool enable if (intervalStr != null && int.TryParse(intervalStr, out int parsedInterval)) maxRetryInterval = parsedInterval; + long maxRateLimitDuration = defaults.MaxRateLimitDuration; + string durationStr = json.GetString("maxRateLimitDuration"); + if (durationStr != null && long.TryParse(durationStr, out long parsedDuration)) + maxRateLimitDuration = parsedDuration; + return new RateLimitConfig( enabled: enabled, maxRetryCount: maxRetryCount, - maxRetryInterval: maxRetryInterval + maxRetryInterval: maxRetryInterval, + maxRateLimitDuration: maxRateLimitDuration ); } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index b4d3857..0895e79 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -5,15 +5,33 @@ namespace Segment.Analytics.Retry { public class RateLimitConfig { + /// Largest Retry-After the client will honour, in seconds. RFC 7231 allows + /// more, but the TAPI agreements cap it here and the other SDKs fix it at this value. + public const int MaxRetryIntervalCeiling = 300; + public bool Enabled { get; } public int MaxRetryCount { get; } public int MaxRetryInterval { get; } - public RateLimitConfig(bool enabled = true, int maxRetryCount = 100, int maxRetryInterval = 300) + /// + /// Wall-clock ceiling, in seconds, on how long one rate-limit episode may keep a + /// batch alive. A last-ditch guard so a pathological Retry-After stream cannot hold + /// a batch forever; is what stops retrying in practice. + /// At the defaults the count is reached first by a wide margin, since + /// MaxRetryCount * MaxRetryIntervalCeiling is well under this. + /// + public long MaxRateLimitDuration { get; } + + public RateLimitConfig( + bool enabled = true, + int maxRetryCount = 100, + int maxRetryInterval = 300, + long maxRateLimitDuration = 43200) { Enabled = enabled; MaxRetryCount = maxRetryCount; MaxRetryInterval = maxRetryInterval; + MaxRateLimitDuration = maxRateLimitDuration; } public RateLimitConfig Validated() => new RateLimitConfig( @@ -21,7 +39,8 @@ public RateLimitConfig(bool enabled = true, int maxRetryCount = 100, int maxRetr // Floored at 1: the count is compared against a fresh state's retry // count, so 0 would drop every batch before it was ever sent. maxRetryCount: Math.Max(1, Math.Min(MaxRetryCount, 1000)), - maxRetryInterval: Math.Max(1, Math.Min(MaxRetryInterval, 3600)) + maxRetryInterval: Math.Max(1, Math.Min(MaxRetryInterval, MaxRetryIntervalCeiling)), + maxRateLimitDuration: Math.Max(0, Math.Min(MaxRateLimitDuration, 604800)) ); } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryState.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryState.cs index f56b6a0..3ce0aef 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryState.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryState.cs @@ -34,6 +34,11 @@ internal class RetryState public PipelineState PipelineState { get; } public long? WaitUntilTime { get; } public int GlobalRetryCount { get; } + + /// When the current rate-limit episode began, for MaxRateLimitDuration. + /// Null outside an episode; cleared on the first success. + public long? RateLimitStartTime { get; } + public Dictionary BatchMetadata { get; } private static readonly Dictionary s_emptyMetadata = @@ -43,11 +48,13 @@ public RetryState( PipelineState pipelineState = PipelineState.Ready, long? waitUntilTime = null, int globalRetryCount = 0, - Dictionary batchMetadata = null) + Dictionary batchMetadata = null, + long? rateLimitStartTime = null) { PipelineState = pipelineState; WaitUntilTime = waitUntilTime; GlobalRetryCount = globalRetryCount; + RateLimitStartTime = rateLimitStartTime; BatchMetadata = batchMetadata ?? s_emptyMetadata; } @@ -63,13 +70,18 @@ public RetryState With( long? waitUntilTime = null, bool clearWaitUntilTime = false, int? globalRetryCount = null, - Dictionary batchMetadata = null) + Dictionary batchMetadata = null, + long? rateLimitStartTime = null, + bool clearRateLimitStartTime = false) { return new RetryState( pipelineState: pipelineState ?? PipelineState, waitUntilTime: clearWaitUntilTime ? null : (waitUntilTime ?? WaitUntilTime), globalRetryCount: globalRetryCount ?? GlobalRetryCount, - batchMetadata: batchMetadata ?? BatchMetadata + batchMetadata: batchMetadata ?? BatchMetadata, + rateLimitStartTime: clearRateLimitStartTime + ? null + : (rateLimitStartTime ?? RateLimitStartTime) ); } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 7f77de7..edfc25e 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -37,7 +37,8 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) pipelineState: PipelineState.Ready, clearWaitUntilTime: true, globalRetryCount: 0, - batchMetadata: RemoveFromMetadata(state, response.BatchFile) + batchMetadata: RemoveFromMetadata(state, response.BatchFile), + clearRateLimitStartTime: true ); } @@ -96,13 +97,29 @@ public Tuple ShouldUploadBatch(RetryState state, str && clearedState.GlobalRetryCount >= _config.RateLimitConfig.MaxRetryCount) { RetryState resetState = clearedState - .With(globalRetryCount: 0) + .With(globalRetryCount: 0, clearRateLimitStartTime: true) .RemoveBatch(batchFile); return Tuple.Create( UploadDecision.DropBatch(DropReason.MaxRetriesExceeded), resetState); } + // Check 2b: how long this rate-limit episode has run. A last-ditch guard so a + // pathological Retry-After stream cannot hold a batch indefinitely; at the + // defaults Check 2 is reached long before this. + if (_config.RateLimitConfig.Enabled + && clearedState.RateLimitStartTime.HasValue + && currentTime - clearedState.RateLimitStartTime.Value + >= _config.RateLimitConfig.MaxRateLimitDuration * 1000) + { + RetryState resetState = clearedState + .With(globalRetryCount: 0, clearRateLimitStartTime: true) + .RemoveBatch(batchFile); + return Tuple.Create( + UploadDecision.DropBatch(DropReason.MaxDurationExceeded), + resetState); + } + // Check 3: Per-batch metadata BatchMetadata metadata; if (clearedState.BatchMetadata.TryGetValue(batchFile, out metadata)) @@ -185,7 +202,10 @@ private RetryState HandleRateLimitResponse(RetryState state, ResponseInfo respon return state.With( pipelineState: PipelineState.RateLimited, waitUntilTime: waitUntilTimeMs, - globalRetryCount: state.GlobalRetryCount + 1 + globalRetryCount: state.GlobalRetryCount + 1, + // Stamped on the first rate-limited response of an episode and left alone + // afterwards, so MaxRateLimitDuration measures the whole episode. + rateLimitStartTime: state.RateLimitStartTime ?? currentTime ); } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs index 7a8cdff..a57d506 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs @@ -52,6 +52,8 @@ private static JsonObject Serialize(RetryState state) }; if (state.WaitUntilTime.HasValue) root["waitUntilTime"] = state.WaitUntilTime.Value; + if (state.RateLimitStartTime.HasValue) + root["rateLimitStartTime"] = state.RateLimitStartTime.Value; if (state.BatchMetadata.Count > 0) { @@ -83,6 +85,7 @@ private static RetryState Deserialize(JsonObject root) pipelineState = PipelineState.RateLimited; long? waitUntilTime = ReadNullableLong(root, "waitUntilTime"); + long? rateLimitStartTime = ReadNullableLong(root, "rateLimitStartTime"); int globalRetryCount = ReadInt(root, "globalRetryCount"); var batchMetadata = new Dictionary(); @@ -101,7 +104,7 @@ private static RetryState Deserialize(JsonObject root) } } - return new RetryState(pipelineState, waitUntilTime, globalRetryCount, batchMetadata); + return new RetryState(pipelineState, waitUntilTime, globalRetryCount, batchMetadata, rateLimitStartTime); } private static int ReadInt(JsonObject json, string key) diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index 4c754f9..487f68e 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -40,7 +40,7 @@ public class EventPipeline: IEventPipeline internal const string UploadSig = "#!upload"; - public EventPipeline( + internal EventPipeline( Analytics analytics, string logTag, string apiKey, diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index a5328e0..d949304 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -51,7 +51,7 @@ public class SyncEventPipeline: IEventPipeline internal int _flushTimeout = -1; internal CancellationToken _flushCancellationToken = CancellationToken.None; - public SyncEventPipeline( + internal SyncEventPipeline( Analytics analytics, string logTag, string apiKey, diff --git a/CHANGELOG.md b/CHANGELOG.md index 85249bd..07583e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,8 @@ rejected. Unity WebGL builds must also add them to the CORS - Send `X-Retry-Count` on retries, so the server can distinguish a retry from a first attempt. - `HttpConfig` is now a settable property on `Configuration` rather than a constructor parameter, so retry behavior can be configured after construction. For mobile targets, CDN settings replace `Configuration.HttpConfig` when they are present. - `Retry-After` is honoured on every retryable status rather than 429 alone, which brings 529 in through the generic 5xx rule. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, capped at `MaxRetryInterval`. +- New `RateLimitConfig.MaxRateLimitDuration` (default 12 hours) bounds how long a single rate-limit episode can keep a batch alive. Every other Segment SDK already had this; C# bounded the rate-limit path by a retry count alone. The count still stops retrying in practice — at the defaults it is reached long before the duration. +- `MaxRetryInterval` is now capped at 300s rather than 3600s, matching the fixed 300s ceiling in the other SDKs. - 511 is dropped rather than retried: it asks the client to authenticate, which this library cannot do. - 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 the HTTP client already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom host values. - `RateLimitConfig.MaxRetryCount` and `BackoffConfig.MaxRetryCount` are floored at 1. A configured 0 previously dropped every batch before it was ever sent. diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 7156dcc..688a239 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -103,6 +103,33 @@ public void SyncEventPipeline_WithoutHttpConfig_RetriesByDefault() Assert.False(pipeline._retryStateMachine.IsLegacyMode); } + [Fact] + public void RateLimitCountIsReachedLongBeforeTheDurationBudget() + { + // MaxRateLimitDuration is a last-ditch guard, not the working limit: the + // count is what should stop retrying at the defaults. If this ever inverts, + // batches start dying on a 12h timer instead of a countable number of tries. + var rateLimit = new RateLimitConfig(); + + long worstCaseSeconds = + (long)rateLimit.MaxRetryCount * RateLimitConfig.MaxRetryIntervalCeiling; + + Assert.True( + worstCaseSeconds < rateLimit.MaxRateLimitDuration, + $"count trips after at most {worstCaseSeconds}s but the duration budget is " + + $"{rateLimit.MaxRateLimitDuration}s; the duration should never be reached first"); + } + + [Fact] + public void RetryAfterIsCappedAtFiveMinutes() + { + // Other SDKs fix this at 300s; C# allowed configuring up to 3600s. + var validated = new RateLimitConfig(maxRetryInterval: 3600).Validated(); + + Assert.Equal(RateLimitConfig.MaxRetryIntervalCeiling, validated.MaxRetryInterval); + Assert.Equal(300, validated.MaxRetryInterval); + } + [Fact] public void DefaultBackoffShapeMatchesTheOtherSdks() { diff --git a/Tests/Retry/HttpConfigParserTest.cs b/Tests/Retry/HttpConfigParserTest.cs index 19fff0c..c59e511 100644 --- a/Tests/Retry/HttpConfigParserTest.cs +++ b/Tests/Retry/HttpConfigParserTest.cs @@ -87,7 +87,8 @@ public void Parse_ClampsValues() HttpConfig config = HttpConfigParser.Parse(json); Assert.Equal(1000, config.RateLimitConfig.MaxRetryCount); - Assert.Equal(3600, config.RateLimitConfig.MaxRetryInterval); + // Retry-After is capped at 300s, matching the other SDKs; it used to allow 3600. + Assert.Equal(300, config.RateLimitConfig.MaxRetryInterval); Assert.Equal(60.0, config.BackoffConfig.BaseBackoffInterval); Assert.Equal(3600, config.BackoffConfig.MaxBackoffInterval); } diff --git a/Tests/Retry/RetryStateMachineTest.cs b/Tests/Retry/RetryStateMachineTest.cs index 8ea18df..9d8df50 100644 --- a/Tests/Retry/RetryStateMachineTest.cs +++ b/Tests/Retry/RetryStateMachineTest.cs @@ -17,10 +17,15 @@ private RetryStateMachine CreateMachine( bool backoffEnabled = true, int maxRetryCount = 100, int maxRetryInterval = 300, - FakeTimeProvider timeProvider = null) + FakeTimeProvider timeProvider = null, + long maxRateLimitDuration = 43200) { var config = new RetryConfig( - new RateLimitConfig(enabled: rateLimitEnabled, maxRetryCount: maxRetryCount, maxRetryInterval: maxRetryInterval), + new RateLimitConfig( + enabled: rateLimitEnabled, + maxRetryCount: maxRetryCount, + maxRetryInterval: maxRetryInterval, + maxRateLimitDuration: maxRateLimitDuration), new BackoffConfig(enabled: backoffEnabled, maxRetryCount: maxRetryCount) ); return new RetryStateMachine(config, timeProvider ?? new FakeTimeProvider(), new Random(42)); @@ -480,5 +485,62 @@ public void GetRetryCount_GlobalHigher_ReturnsGlobal() Assert.Equal(10, machine.GetRetryCount(state, "batch1.json")); } + + [Fact] + public void RateLimitEpisodeIsBoundedByMaxRateLimitDuration() + { + // A pathological Retry-After stream used to be bounded only by a retry count; + // this is the wall-clock backstop the other SDKs have had all along. + var clock = new FakeTimeProvider(); + var machine = CreateMachine( + maxRetryCount: 1000, timeProvider: clock, maxRateLimitDuration: 60); + + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 5, "b.json", clock.CurrentTimeMillis())); + Assert.Equal(clock.CurrentTimeMillis(), state.RateLimitStartTime); + + clock.Time += 61_000; + Tuple decision = machine.ShouldUploadBatch(state, "b.json"); + + Assert.IsType(decision.Item1); + Assert.Equal( + DropReason.MaxDurationExceeded, + ((UploadDecision.DropBatchDecision)decision.Item1).Reason); + Assert.Null(decision.Item2.RateLimitStartTime); + } + + [Fact] + public void RateLimitStartTimeSurvivesAFurtherRateLimitedResponse() + { + // Stamped once per episode: re-stamping on every 429 would let the budget + // never expire under sustained rate limiting, which is the case it exists for. + var clock = new FakeTimeProvider(); + var machine = CreateMachine(timeProvider: clock); + long began = clock.CurrentTimeMillis(); + + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 5, "b.json", began)); + clock.Time += 30_000; + state = machine.HandleResponse( + state, new ResponseInfo(429, 5, "b.json", clock.CurrentTimeMillis())); + + Assert.Equal(began, state.RateLimitStartTime); + } + + [Fact] + public void SuccessEndsTheRateLimitEpisode() + { + var clock = new FakeTimeProvider(); + var machine = CreateMachine(timeProvider: clock); + + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 5, "b.json", clock.CurrentTimeMillis())); + Assert.NotNull(state.RateLimitStartTime); + + state = machine.HandleResponse( + state, new ResponseInfo(200, null, "b.json", clock.CurrentTimeMillis())); + + Assert.Null(state.RateLimitStartTime); + } } } From f70f82d5d1ea91809e4970876b0ece5a9e630be4 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:23:20 -0400 Subject: [PATCH 14/15] Merge status code overrides over the defaults instead of replacing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Segment/Analytics/Retry/RetryConfig.cs | 22 +++++++-- CHANGELOG.md | 3 +- Tests/Retry/ConfigurationHttpConfigTest.cs | 46 +++++++++++++++++++ Tests/Retry/HttpConfigParserTest.cs | 10 +++- 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 0895e79..efe0b78 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -78,10 +78,19 @@ public BackoffConfig( Default4xxBehavior = default4xxBehavior; Default5xxBehavior = default5xxBehavior; UnknownCodeBehavior = unknownCodeBehavior; - // Copied because the property is public: sharing the static default would let - // one caller's mutation corrupt every BackoffConfig built afterwards. - StatusCodeOverrides = new Dictionary( - statusCodeOverrides ?? DefaultStatusCodeOverrides); + // Merged over the defaults, not substituted for them. Replacing meant that + // overriding one status silently changed seven others: 408, 410, 429 and + // 460 stopped being retried, and 511 fell through to Default5xxBehavior + // and started being retried, which is the one thing it must never do. + // Copied rather than aliased because the property is public, so sharing + // the static default would let one caller's mutation corrupt every + // BackoffConfig built afterwards. + StatusCodeOverrides = new Dictionary(DefaultStatusCodeOverrides); + if (statusCodeOverrides != null) + { + foreach (KeyValuePair kvp in statusCodeOverrides) + StatusCodeOverrides[kvp.Key] = kvp.Value; + } } public BackoffConfig Validated() => new BackoffConfig( @@ -89,7 +98,10 @@ public BackoffConfig( maxRetryCount: Math.Max(1, Math.Min(MaxRetryCount, 1000)), baseBackoffInterval: Math.Max(0.1, Math.Min(BaseBackoffInterval, 60.0)), maxBackoffInterval: Math.Max(1, Math.Min(MaxBackoffInterval, 3600)), - maxTotalBackoffDuration: Math.Max(0, Math.Min(MaxTotalBackoffDuration, 604800)), + // Floored at 1 for the same reason as maxRetryCount: ExceedsMaxDuration + // compares elapsed time against this, so 0 meant "no budget" — the batch + // was abandoned on its second attempt — rather than "no cap". + maxTotalBackoffDuration: Math.Max(1, Math.Min(MaxTotalBackoffDuration, 604800)), jitterPercent: Math.Max(0, Math.Min(JitterPercent, 50)), default4xxBehavior: Default4xxBehavior, default5xxBehavior: Default5xxBehavior, diff --git a/CHANGELOG.md b/CHANGELOG.md index 07583e4..d3b75a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,4 +50,5 @@ rejected. Unity WebGL builds must also add them to the CORS - 511 is dropped rather than retried: it asks the client to authenticate, which this library cannot do. - 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 the HTTP client already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom host values. - `RateLimitConfig.MaxRetryCount` and `BackoffConfig.MaxRetryCount` are floored at 1. A configured 0 previously dropped every batch before it was ever sent. -- `BackoffConfig.StatusCodeOverrides` is copied rather than held by reference, so mutating the caller's dictionary no longer changes a live config. +- `BackoffConfig.StatusCodeOverrides` is merged over the built-in defaults rather than replacing them, and is copied rather than held by reference. Previously, supplying an override for one status silently changed seven others: 408, 410, 429 and 460 stopped being retried, and 511 fell through to `Default5xxBehavior` and started being retried. A CDN settings payload whose overrides were all unparseable had the same effect. +- `BackoffConfig.MaxTotalBackoffDuration` is floored at 1 second. A configured 0 meant "no budget" — the batch was abandoned on its second attempt — rather than "no cap". diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 688a239..a4c5077 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using Moq; using Segment.Analytics; using Segment.Analytics.Retry; @@ -103,6 +104,51 @@ public void SyncEventPipeline_WithoutHttpConfig_RetriesByDefault() Assert.False(pipeline._retryStateMachine.IsLegacyMode); } + [Fact] + public void StatusCodeOverridesMergeOverTheDefaultsRatherThanReplacingThem() + { + // Overriding one status used to drop the defaults for every other: 408, + // 410, 429 and 460 stopped being retried, and 511 fell through to + // Default5xxBehavior and started being retried. + var config = new BackoffConfig( + statusCodeOverrides: new Dictionary + { + { 503, RetryBehavior.Drop } + }); + + Assert.Equal(RetryBehavior.Drop, config.StatusCodeOverrides[503]); + Assert.Equal(RetryBehavior.Retry, config.StatusCodeOverrides[408]); + Assert.Equal(RetryBehavior.Retry, config.StatusCodeOverrides[410]); + Assert.Equal(RetryBehavior.Retry, config.StatusCodeOverrides[429]); + Assert.Equal(RetryBehavior.Retry, config.StatusCodeOverrides[460]); + Assert.Equal(RetryBehavior.Drop, config.StatusCodeOverrides[501]); + Assert.Equal(RetryBehavior.Drop, config.StatusCodeOverrides[505]); + Assert.Equal(RetryBehavior.Drop, config.StatusCodeOverrides[511]); + } + + [Fact] + public void AnOverrideCanStillContradictADefault() + { + // Merging must not make the defaults unopposable. + var config = new BackoffConfig( + statusCodeOverrides: new Dictionary + { + { 429, RetryBehavior.Drop } + }); + + Assert.Equal(RetryBehavior.Drop, config.StatusCodeOverrides[429]); + } + + [Fact] + public void MaxTotalBackoffDurationOfZeroDoesNotAbandonOnTheSecondAttempt() + { + // ExceedsMaxDuration compares elapsed time against this, so an unfloored + // 0 read as "no budget" rather than "no cap". + var validated = new BackoffConfig(maxTotalBackoffDuration: 0).Validated(); + + Assert.True(validated.MaxTotalBackoffDuration >= 1); + } + [Fact] public void RateLimitCountIsReachedLongBeforeTheDurationBudget() { diff --git a/Tests/Retry/HttpConfigParserTest.cs b/Tests/Retry/HttpConfigParserTest.cs index c59e511..b8791e4 100644 --- a/Tests/Retry/HttpConfigParserTest.cs +++ b/Tests/Retry/HttpConfigParserTest.cs @@ -75,7 +75,15 @@ public void Parse_InvalidStatusCodeOverrides_Filtered() "{\"backoffConfig\":{\"statusCodeOverrides\":{\"abc\":\"retry\",\"999\":\"retry\",\"200\":\"invalid\"}}}"); HttpConfig config = HttpConfigParser.Parse(json); - Assert.Empty(config.BackoffConfig.StatusCodeOverrides); + // The unusable entries are dropped. + Assert.False(config.BackoffConfig.StatusCodeOverrides.ContainsKey(999)); + Assert.False(config.BackoffConfig.StatusCodeOverrides.ContainsKey(200)); + + // And the built-in defaults survive. This used to assert the dictionary + // was empty, which meant a settings payload of nothing but junk wiped + // them — leaving 511 to fall through to Default5xxBehavior and be retried. + Assert.Equal(RetryBehavior.Drop, config.BackoffConfig.StatusCodeOverrides[511]); + Assert.Equal(RetryBehavior.Retry, config.BackoffConfig.StatusCodeOverrides[429]); } [Fact] From 66848a77071be1bf8cd2bdf25d34cb2c1b65e9ad Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:34:51 -0400 Subject: [PATCH 15/15] Cover RateLimitStartTime persistence 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. --- Tests/Retry/RetryStateStorageTest.cs | 36 ++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/Tests/Retry/RetryStateStorageTest.cs b/Tests/Retry/RetryStateStorageTest.cs index 7dad27c..dfb89c9 100644 --- a/Tests/Retry/RetryStateStorageTest.cs +++ b/Tests/Retry/RetryStateStorageTest.cs @@ -51,6 +51,42 @@ public void RoundTrip_RateLimitedState() Assert.Equal(3, loaded.GlobalRetryCount); } + [Fact] + public void RoundTrip_RateLimitStartTime() + { + // MaxRateLimitDuration measures from this, so losing it across a restart + // would restart the episode clock and let a batch outlive its budget. + var state = new RetryState( + pipelineState: PipelineState.RateLimited, + waitUntilTime: 1_700_000_030_000, + globalRetryCount: 3, + rateLimitStartTime: 1_700_000_000_000); + + RetryStateStorage.SaveRetryState(_storage.Object, state); + RetryState loaded = RetryStateStorage.LoadRetryState(_storage.Object); + + Assert.Equal(1_700_000_000_000, loaded.RateLimitStartTime); + Assert.Equal(1_700_000_030_000, loaded.WaitUntilTime); + Assert.Equal(3, loaded.GlobalRetryCount); + } + + [Fact] + public void LoadRetryState_StateWrittenBeforeRateLimitStartTimeExisted() + { + // State persisted by 2.6.0 has no such key; it must load as null rather + // than failing or defaulting to the epoch, which would read as an episode + // that started in 1970 and expire every batch immediately. + _storage + .Setup(s => s.Read(StorageConstants.RetryState)) + .Returns("{\"pipelineState\":\"RateLimited\",\"globalRetryCount\":2,\"waitUntilTime\":1700000030000}"); + + RetryState loaded = RetryStateStorage.LoadRetryState(_storage.Object); + + Assert.Null(loaded.RateLimitStartTime); + Assert.Equal(PipelineState.RateLimited, loaded.PipelineState); + Assert.Equal(2, loaded.GlobalRetryCount); + } + [Fact] public void RoundTrip_WithBatchMetadata() {