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 3056e8a..6cfeeb8 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -209,11 +209,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 +219,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..b1e4cd8 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/HTTPClient.cs @@ -188,7 +188,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; } } 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..e64a53b 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -234,11 +234,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 +244,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/Tests/Retry/RetryAfterDeleteBatchTest.cs b/Tests/Retry/RetryAfterDeleteBatchTest.cs new file mode 100644 index 0000000..8faf03d --- /dev/null +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -0,0 +1,74 @@ +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)); + } + } +} 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]