From 596064839e246dbfc6eb0946e1cf133d4a5736df Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 2 Sep 2026 19:38:14 -0400 Subject: [PATCH 1/4] 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. --- .../Analytics/Retry/RetryStateMachine.cs | 14 +++- .../Analytics/Utilities/EventPipeline.cs | 6 +- .../Analytics/Utilities/RetryAfterParser.cs | 38 +++++++++ .../Analytics/Utilities/SyncEventPipeline.cs | 6 +- Tests/Retry/RetryAfterParserTest.cs | 81 +++++++++++++++++++ Tests/Retry/RetryStateMachineTest.cs | 66 +++++++++++++++ 6 files changed, 199 insertions(+), 12 deletions(-) create mode 100644 Analytics-CSharp/Segment/Analytics/Utilities/RetryAfterParser.cs create mode 100644 Tests/Retry/RetryAfterParserTest.cs diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 3538bc8..2777808 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -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); diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index 3056e8a..ecdd89e 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) { 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..40dcdde 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) { 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] From bd0b7dbfc3ac7e3402355d0db49a06448f07d07e Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 13:05:45 -0400 Subject: [PATCH 2/4] Keep the batch when Retry-After routes it to the rate-limit path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Analytics/Retry/RetryStateMachine.cs | 12 ++++- Tests/Retry/RetryAfterDeleteBatchTest.cs | 49 +++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 Tests/Retry/RetryAfterDeleteBatchTest.cs diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 2777808..9ecee4b 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -153,8 +153,16 @@ public bool ShouldDeleteBatch(int statusCode) return !_config.RateLimitConfig.Enabled; RetryBehavior behavior = ResolveStatusCodeBehavior(statusCode); - if (behavior == RetryBehavior.Retry && !_config.BackoffConfig.Enabled) - return true; + if (behavior == RetryBehavior.Retry) + { + // Rate limiting handles retryable codes that carry Retry-After, so the batch + // must be kept for the retry that HandleResponse has just scheduled. + if (_config.RateLimitConfig.Enabled) + return false; + + // Retryable, but neither rate limiting nor backoff is on: nothing will retry it. + return !_config.BackoffConfig.Enabled; + } return behavior == RetryBehavior.Drop; } diff --git a/Tests/Retry/RetryAfterDeleteBatchTest.cs b/Tests/Retry/RetryAfterDeleteBatchTest.cs new file mode 100644 index 0000000..8c147bc --- /dev/null +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -0,0 +1,49 @@ +using Segment.Analytics.Retry; +using Xunit; + +namespace Tests.Retry +{ + /// + /// Regression: a retryable status carrying Retry-After routes to the rate-limit path, + /// so the batch must NOT also be deleted — otherwise the pipeline stalls waiting to + /// retry a batch that no longer exists. Mirrors swift's shouldDropBatch, which returns + /// false for retryable codes whenever rate limiting is enabled. + /// + 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_WithRateLimitEnabled_IsNotDeleted(int status) + { + Assert.False(RateLimitOnlyMachine().ShouldDeleteBatch(status)); + } + + [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)); + } + + [Fact] + public void NonRetryableStatus_IsStillDeleted() + { + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(400)); + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(501)); + } + } +} From 14b823821c004938ab9d096bf25dedf13669597f Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 13:10:40 -0400 Subject: [PATCH 3/4] Base the keep-or-delete decision on Retry-After, not just config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit kept a retryable batch whenever rate limiting was enabled, which was too broad: a 500 with no Retry-After and backoff disabled was also kept, so the file was re-uploaded even though nothing had scheduled a retry. The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects exactly one request and saw two. ShouldDeleteBatch now takes the same retryAfterSeconds value handed to HandleResponse, so the two agree on whether the response actually took the rate-limit path. A retryable status keeps its batch only when it carries a usable Retry-After and rate limiting is on; otherwise only backoff can retry it, and with backoff off the batch is dropped as before. The single-argument overload is retained. 232 tests pass. --- .../Analytics/Retry/RetryStateMachine.cs | 17 +++++++--- .../Analytics/Utilities/EventPipeline.cs | 2 +- .../Analytics/Utilities/SyncEventPipeline.cs | 2 +- Tests/Retry/RetryAfterDeleteBatchTest.cs | 34 +++++++++++++------ 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 9ecee4b..853d9ed 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -141,7 +141,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; @@ -155,12 +162,12 @@ public bool ShouldDeleteBatch(int statusCode) RetryBehavior behavior = ResolveStatusCodeBehavior(statusCode); if (behavior == RetryBehavior.Retry) { - // Rate limiting handles retryable codes that carry Retry-After, so the batch - // must be kept for the retry that HandleResponse has just scheduled. - if (_config.RateLimitConfig.Enabled) + // 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; - // Retryable, but neither rate limiting nor backoff is on: nothing will retry it. + // Otherwise only backoff can retry it; with backoff off, nothing will. return !_config.BackoffConfig.Enabled; } diff --git a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs index ecdd89e..6cfeeb8 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/EventPipeline.cs @@ -219,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/SyncEventPipeline.cs b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs index 40dcdde..e64a53b 100644 --- a/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs +++ b/Analytics-CSharp/Segment/Analytics/Utilities/SyncEventPipeline.cs @@ -244,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 index 8c147bc..fc667ec 100644 --- a/Tests/Retry/RetryAfterDeleteBatchTest.cs +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -4,10 +4,9 @@ namespace Tests.Retry { /// - /// Regression: a retryable status carrying Retry-After routes to the rate-limit path, - /// so the batch must NOT also be deleted — otherwise the pipeline stalls waiting to - /// retry a batch that no longer exists. Mirrors swift's shouldDropBatch, which returns - /// false for retryable codes whenever rate limiting is enabled. + /// 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 { @@ -21,9 +20,24 @@ private static RetryStateMachine RateLimitOnlyMachine() => [InlineData(529)] [InlineData(408)] [InlineData(410)] - public void RetryableStatus_WithRateLimitEnabled_IsNotDeleted(int status) + public void RetryableStatus_WithRetryAfter_IsKept(int status) { - Assert.False(RateLimitOnlyMachine().ShouldDeleteBatch(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] @@ -36,14 +50,14 @@ public void RetryAfter_RateLimitsPipelineAndKeepsBatch() Assert.Equal(PipelineState.RateLimited, state.PipelineState); Assert.Equal(31000, state.WaitUntilTime); - Assert.False(machine.ShouldDeleteBatch(503)); + Assert.False(machine.ShouldDeleteBatch(503, 30)); } [Fact] - public void NonRetryableStatus_IsStillDeleted() + public void NonRetryableStatus_IsDeletedEvenWithRetryAfter() { - Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(400)); - Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(501)); + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(400, 30)); + Assert.True(RateLimitOnlyMachine().ShouldDeleteBatch(501, 30)); } } } From 001b4435afaed7b6330a558b5490ecf8cab901d7 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 17:20:57 -0400 Subject: [PATCH 4/4] 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. --- .../Segment/Analytics/Retry/RetryStateMachine.cs | 7 ++++--- .../Segment/Analytics/Utilities/HTTPClient.cs | 3 ++- Tests/Retry/RetryAfterDeleteBatchTest.cs | 11 +++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 853d9ed..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, @@ -153,7 +153,8 @@ 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) 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/Tests/Retry/RetryAfterDeleteBatchTest.cs b/Tests/Retry/RetryAfterDeleteBatchTest.cs index fc667ec..8faf03d 100644 --- a/Tests/Retry/RetryAfterDeleteBatchTest.cs +++ b/Tests/Retry/RetryAfterDeleteBatchTest.cs @@ -53,6 +53,17 @@ public void RetryAfter_RateLimitsPipelineAndKeepsBatch() 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() {