diff --git a/Analytics-CSharp/Segment/Analytics/Configuration.cs b/Analytics-CSharp/Segment/Analytics/Configuration.cs index 79de436..c5fba22 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,24 @@ private set public IEventPipelineProvider EventPipelineProvider { get; } + /// + /// HTTP retry configuration for rate limiting and exponential backoff. Defaults to + /// 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 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 behavior of + /// analytics-kotlin and analytics-swift. + /// + /// + public HttpConfig HttpConfig { get; set; } + /// /// Configuration that analytics can use /// diff --git a/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs b/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs index 8412944..43e8807 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/HttpConfigParser.cs @@ -46,20 +46,28 @@ 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; + 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 ); } @@ -68,27 +76,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 5be1331..efe0b78 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -3,27 +3,48 @@ namespace Segment.Analytics.Retry { - internal class RateLimitConfig + 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 = false, 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( enabled: Enabled, - maxRetryCount: Math.Max(0, Math.Min(MaxRetryCount, 1000)), - maxRetryInterval: Math.Max(1, Math.Min(MaxRetryInterval, 3600)) + // 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, MaxRetryIntervalCeiling)), + maxRateLimitDuration: Math.Max(0, Math.Min(MaxRateLimitDuration, 604800)) ); } - internal class BackoffConfig + public class BackoffConfig { public bool Enabled { get; } public int MaxRetryCount { get; } @@ -37,10 +58,10 @@ internal 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, @@ -57,15 +78,30 @@ public BackoffConfig( Default4xxBehavior = default4xxBehavior; Default5xxBehavior = default5xxBehavior; UnknownCodeBehavior = unknownCodeBehavior; - StatusCodeOverrides = 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( 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)), + // 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, @@ -93,7 +129,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 } }; } @@ -109,7 +149,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/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 3538bc8..edfc25e 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -37,19 +37,33 @@ 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 ); } + // 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) 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); } - 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); @@ -83,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)) @@ -131,7 +161,14 @@ 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; @@ -139,12 +176,22 @@ public bool ShouldDeleteBatch(int statusCode) 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; 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; } @@ -155,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/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..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, @@ -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, @@ -69,7 +69,9 @@ internal EventPipeline( Running = false; var retryConfig = httpConfig != null - ? new RetryConfig(httpConfig.RateLimitConfig, httpConfig.BackoffConfig) + // 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); _retryState = RetryStateStorage.LoadRetryState(_storage); @@ -78,7 +80,7 @@ internal 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); } @@ -209,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) { @@ -223,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/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/HTTPClient.cs b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs index 22b0168..d12dedc 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; @@ -123,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 + // behavior 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) { @@ -188,6 +203,8 @@ public class Response /// /// A convenient method to check if the http request is successful /// + // 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; } } @@ -242,6 +259,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 4657be9..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, @@ -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, @@ -86,7 +86,9 @@ internal SyncEventPipeline( _flushCancellationToken = flushCancellationToken ?? CancellationToken.None; var retryConfig = httpConfig != null - ? new RetryConfig(httpConfig.RateLimitConfig, httpConfig.BackoffConfig) + // 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); _retryState = RetryStateStorage.LoadRetryState(_storage); @@ -95,7 +97,7 @@ internal 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); } @@ -234,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) { @@ -248,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/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/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..d3b75a8 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,54 @@ +# 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 + +### 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 behavior. + +To keep the old behavior, 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` +(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 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. +- `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/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/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs new file mode 100644 index 0000000..a4c5077 --- /dev/null +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -0,0 +1,248 @@ +using System; +using System.Collections.Generic; +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_RetriesByDefault() + { + // Server-side users get no CDN settings, so a null HttpConfig has to mean + // "retry with the defaults", not "no retry behavior 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); + } + + [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_RetriesByDefault() + { + Analytics analytics = CreateAnalytics(null); + + var pipeline = (SyncEventPipeline)new SyncEventPipelineProvider().Create(analytics, "key"); + + 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() + { + // 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() + { + // 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] + 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() + { + 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() + { + Analytics analytics = CreateAnalytics( + new HttpConfig(rateLimitConfig: new RateLimitConfig(enabled: true))); + + var pipeline = (SyncEventPipeline)new SyncEventPipelineProvider().Create(analytics, "key"); + + Assert.False(pipeline._retryStateMachine.IsLegacyMode); + } + } +} diff --git a/Tests/Retry/HttpConfigParserTest.cs b/Tests/Retry/HttpConfigParserTest.cs index 89ff4cf..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] @@ -87,7 +95,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); } @@ -101,7 +110,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/RetryAfterDeleteBatchTest.cs b/Tests/Retry/RetryAfterDeleteBatchTest.cs new file mode 100644 index 0000000..8a3d12a --- /dev/null +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -0,0 +1,98 @@ +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(204)] + public void SuccessStatuses_AreDeleted(int status) + { + // 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() + { + 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..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)); @@ -97,8 +102,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 +119,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] @@ -371,6 +381,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] @@ -409,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); + } } } 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() { 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" } }