From bde413303f37d7e9188d30f9fbb44f2842fdbbe2 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 17:37:50 -0400 Subject: [PATCH 1/9] Bound the rate-limit budget at 5 minutes instead of 12 hours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 12 hour default was a backstop on the assumption a retry count would stop us reaching it. Across the SDKs rate-limited attempts are deliberately uncounted, so a duration is what actually bounds that path — C# was the only one with a count at all, and it sat at 100. Five minutes matches the counted path's ~4 minute worst case. MaxRetryInterval drops to 60s. At 300s it equalled the whole budget, so one sleep consumed it and the rate-limit path gave a single attempt. The wait is clamped to the end of the episode's budget, since ShouldUploadBatch checks elapsed time before waiting. One test assertion is deliberately inverted rather than adjusted: RateLimitCountIsReachedLongBeforeTheDurationBudget asserted that the count trips first and the duration is unreachable. That was the right invariant when the duration was 12 hours; now the duration is the operative limit and the count is the backstop, so it asserts the reverse and says why. 264 unit tests and the 82-test e2e suite pass. The e2e suite failed 6 tests on an earlier run under heavy machine load and passed clean on a re-run at normal speed — it is timing-sensitive, which is worth knowing for CI. --- .../Segment/Analytics/Retry/RetryConfig.cs | 18 ++++++------ .../Analytics/Retry/RetryStateMachine.cs | 8 ++++++ CHANGELOG.md | 3 ++ Tests/Retry/ConfigurationHttpConfigTest.cs | 28 +++++++++++-------- Tests/Retry/HttpConfigParserTest.cs | 10 ++++--- 5 files changed, 43 insertions(+), 24 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index efe0b78..313b87c 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -5,9 +5,10 @@ namespace Segment.Analytics.Retry { public class RateLimitConfig { - /// Largest Retry-After the client will honour, in seconds. RFC 7231 allows - /// more, but the TAPI agreements cap it here and the other SDKs fix it at this value. - public const int MaxRetryIntervalCeiling = 300; + /// Largest Retry-After the client will honour, in seconds. Kept well below + /// so the budget buys several attempts rather than + /// one long sleep; at the old 300s a single sleep consumed the whole budget. + public const int MaxRetryIntervalCeiling = 60; public bool Enabled { get; } public int MaxRetryCount { get; } @@ -15,18 +16,17 @@ public class RateLimitConfig /// /// 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. + /// batch alive. Five minutes, in line with the counted-backoff path's ~4 minute + /// worst case. This is the operative limit on that path: rate-limited attempts are + /// deliberately uncounted, so a duration is the only thing bounding them. /// public long MaxRateLimitDuration { get; } public RateLimitConfig( bool enabled = true, int maxRetryCount = 100, - int maxRetryInterval = 300, - long maxRateLimitDuration = 43200) + int maxRetryInterval = MaxRetryIntervalCeiling, + long maxRateLimitDuration = 300) { Enabled = enabled; MaxRetryCount = maxRetryCount; diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index edfc25e..090abe8 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -199,6 +199,14 @@ public bool ShouldDeleteBatch(int statusCode, int? retryAfterSeconds) private RetryState HandleRateLimitResponse(RetryState state, ResponseInfo response, long currentTime) { long waitUntilTimeMs = CalculateWaitUntilTimeMs(response.RetryAfterSeconds, currentTime); + + // Clamped to the end of the budget: ShouldUploadBatch checks elapsed time + // before the wait, so without this a check passing just inside the budget + // would wait a full Retry-After beyond it. + long episodeStart = state.RateLimitStartTime ?? currentTime; + long deadline = episodeStart + (_config.RateLimitConfig.MaxRateLimitDuration * 1000L); + if (waitUntilTimeMs > deadline) + waitUntilTimeMs = deadline; return state.With( pipelineState: PipelineState.RateLimited, waitUntilTime: waitUntilTimeMs, diff --git a/CHANGELOG.md b/CHANGELOG.md index d3b75a8..6a263f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ This file carries the notes that need more than a pull-request title. ## Unreleased +- `RateLimitConfig.MaxRateLimitDuration` now defaults to 5 minutes rather than 12 hours, and `MaxRetryInterval` to 60s rather than 300s. The 12 hour value was a backstop meant to be unreachable, but rate-limited attempts are deliberately uncounted across the SDKs, so a duration is what genuinely bounds that path. Five minutes lines up with the counted-backoff path's ~4 minute worst case; 60s keeps a single `Retry-After` from consuming the whole budget. +- The rate-limit wait is clamped to the end of the episode's budget. `ShouldUploadBatch` checks elapsed time before the wait, so a check passing just inside the budget previously waited a full `Retry-After` beyond it. + ### Behavior change: retries and backoff are on by default Through 2.6.0, rate limiting and exponential backoff were both disabled unless you diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index a4c5077..efca52f 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -150,30 +150,36 @@ public void MaxTotalBackoffDurationOfZeroDoesNotAbandonOnTheSecondAttempt() } [Fact] - public void RateLimitCountIsReachedLongBeforeTheDurationBudget() + public void MaxRateLimitDurationIsTheOperativeLimitNotTheCount() { - // 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. + // This assertion is the inverse of what it was, deliberately. The count used + // to be the working limit with a 12h duration as an unreachable backstop — + // but rate-limited attempts are uncounted across the other SDKs, so the + // duration is what genuinely bounds this path. At 5 minutes against a 60s + // ceiling it now trips first, and the count is the backstop. var rateLimit = new RateLimitConfig(); - long worstCaseSeconds = + long countWouldAllowSeconds = (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"); + rateLimit.MaxRateLimitDuration < countWouldAllowSeconds, + $"duration is {rateLimit.MaxRateLimitDuration}s but the count would allow " + + $"{countWouldAllowSeconds}s; the duration should bound this path"); } [Fact] - public void RetryAfterIsCappedAtFiveMinutes() + public void RetryAfterIsCappedWellBelowTheBudget() { - // Other SDKs fix this at 300s; C# allowed configuring up to 3600s. + // A cap equal to the budget would let one sleep consume it, leaving the + // rate-limit path with a single attempt. var validated = new RateLimitConfig(maxRetryInterval: 3600).Validated(); Assert.Equal(RateLimitConfig.MaxRetryIntervalCeiling, validated.MaxRetryInterval); - Assert.Equal(300, validated.MaxRetryInterval); + Assert.Equal(60, validated.MaxRetryInterval); + Assert.True( + validated.MaxRetryInterval * 4 <= validated.MaxRateLimitDuration, + "the budget should buy at least a few attempts, not one"); } [Fact] diff --git a/Tests/Retry/HttpConfigParserTest.cs b/Tests/Retry/HttpConfigParserTest.cs index b8791e4..5abb54b 100644 --- a/Tests/Retry/HttpConfigParserTest.cs +++ b/Tests/Retry/HttpConfigParserTest.cs @@ -50,11 +50,12 @@ public void Parse_BackoffConfig_ParsesValues() public void Parse_RateLimitConfig_ParsesValues() { var json = JsonUtility.FromJson( - "{\"rateLimitConfig\":{\"maxRetryCount\":\"10\",\"maxRetryInterval\":\"120\"}}"); + "{\"rateLimitConfig\":{\"maxRetryCount\":\"10\",\"maxRetryInterval\":\"45\"}}"); HttpConfig config = HttpConfigParser.Parse(json); Assert.Equal(10, config.RateLimitConfig.MaxRetryCount); - Assert.Equal(120, config.RateLimitConfig.MaxRetryInterval); + // Under the 60s ceiling, so it survives validation unchanged. + Assert.Equal(45, config.RateLimitConfig.MaxRetryInterval); } [Fact] @@ -95,8 +96,9 @@ public void Parse_ClampsValues() HttpConfig config = HttpConfigParser.Parse(json); Assert.Equal(1000, config.RateLimitConfig.MaxRetryCount); - // Retry-After is capped at 300s, matching the other SDKs; it used to allow 3600. - Assert.Equal(300, config.RateLimitConfig.MaxRetryInterval); + // Retry-After is capped at 60s: well below the 5 minute rate-limit budget, so + // the budget buys several attempts rather than one long sleep. + Assert.Equal(60, config.RateLimitConfig.MaxRetryInterval); Assert.Equal(60.0, config.BackoffConfig.BaseBackoffInterval); Assert.Equal(3600, config.BackoffConfig.MaxBackoffInterval); } From 5c883f040807ea7c81bd7b97541fcc4469abe592 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 17:53:09 -0400 Subject: [PATCH 2/9] Rewrite the release notes for a reader seeing them in isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three problems. The notes described changes between states that never shipped, so a customer read that a default moved from 12 hours to 5 minutes when only the 5 minutes was ever released. They referred to other SDKs, which means nothing to someone reading one library's notes. And they had accumulated over several passes into contradictions — Retry-After was documented as capped at both 300s and 60s, and the rate-limit budget as both 12 hours and 5 minutes. Rewritten to describe the behaviour this version has, in a consistent structure: upgrade notes that need action first, then retry handling, then everything else. Entries covering fixes to code that has not shipped are dropped, since there is nothing for a reader to compare against. --- CHANGELOG.md | 66 +++++++++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a263f4..1ab4aaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,19 +6,16 @@ This file carries the notes that need more than a pull-request title. ## Unreleased -- `RateLimitConfig.MaxRateLimitDuration` now defaults to 5 minutes rather than 12 hours, and `MaxRetryInterval` to 60s rather than 300s. The 12 hour value was a backstop meant to be unreachable, but rate-limited attempts are deliberately uncounted across the SDKs, so a duration is what genuinely bounds that path. Five minutes lines up with the counted-backoff path's ~4 minute worst case; 60s keeps a single `Retry-After` from consuming the whole budget. -- The rate-limit wait is clamped to the end of the episode's budget. `ShouldUploadBatch` checks elapsed time before the wait, so a check passing just inside the budget previously waited a full `Retry-After` beyond it. +### Behaviour change: retries and backoff are on by default -### Behavior change: retries and backoff are on by default +Through 2.6.0, rate limiting and exponential backoff were both disabled unless an +`HttpConfig` was supplied 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 retry behaviour described below. -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: +To keep the previous behaviour, disable both explicitly: ```csharp new Configuration("writeKey") @@ -29,29 +26,30 @@ new Configuration("writeKey") } ``` -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. +CDN settings still take precedence: a payload carrying an `httpConfig` key replaces +whatever the pipeline is running with, and a payload without that key leaves the +supplied configuration in effect. -### Upgrade note: new request headers and proxy allowlists +### Upgrade note: new request headers 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". +(HTTP Basic, carrying the write key) and `X-Retry-Count` (on retries only). If +traffic to Segment passes 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. + +### Retry handling + +- A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `RateLimitConfig.MaxRetryInterval` (default 60 seconds). +- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 5 minutes). Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. +- 511 is dropped rather than retried: it asks the client to re-authenticate, which this library cannot do. +- `RetryBehavior`, `RateLimitConfig`, `BackoffConfig` and `HttpConfig` are now public, and `HttpConfig` is a settable property on `Configuration`, so retry behaviour can be configured in code. On mobile targets, CDN settings replace it when present. +- `BackoffConfig.StatusCodeOverrides` is merged over the built-in defaults rather than replacing them, so overriding one status leaves the rest unchanged. +- Retry counts and interval limits are clamped to usable ranges rather than accepted as given. + +### Other changes + +- The write key is sent as an `Authorization: Basic` header. It remains in the request body, so no server-side change is required. +- `X-Retry-Count` is sent on retries, allowing the server to distinguish a retry from a first attempt. +- Only 2xx responses count as a successful upload. A 3xx is reported as a failed upload rather than treated as delivered, and is not retried: a redirect the HTTP client has already declined to follow will not succeed on one. The Segment endpoint does not redirect, so this affects only custom host values. From 1ba78a1f1238883814a1160ce4d57dd17977dfda Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 19:22:53 -0400 Subject: [PATCH 3/9] Stop carrying the rate-limit episode clock across restarts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RateLimitStartTime was persisted as an absolute timestamp and restored on load. That was survivable at a 12 hour budget and is not at 5 minutes: an app closed for longer than the budget now loads an already-expired episode and discards the batch on its first flush, having never retried it while actually running. The clock measures how long this process has spent retrying, and time while the process was not running is not that. It is now in-memory only — neither written nor read. The retry counts still persist, so a batch cannot be retried indefinitely across restarts; a relaunch gets a fresh duration budget but inherits the counted one. This only affects targets that persist state at all. Server-side use generally swaps the disk store for an in-memory one, where the field was already per-process. Also corrects a comment I wrote in the previous commit. It described the duration as a last-ditch guard reached long after the count — true at 12 hours, and the inverse of what the same commit made true. The duration is now what stops retrying and the count is the backstop, which is what the test one file over asserts. 264 unit tests and the 82-test e2e suite pass. --- .../Segment/Analytics/Retry/RetryStateMachine.cs | 6 +++--- .../Segment/Analytics/Retry/RetryStateStorage.cs | 14 ++++++++++---- Tests/Retry/RetryStateStorageTest.cs | 10 ++++++---- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 090abe8..05bdcac 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -104,9 +104,9 @@ public Tuple ShouldUploadBatch(RetryState state, str 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. + // Check 2b: how long this rate-limit episode has run. At the defaults this + // is what stops retrying — rate-limited attempts are uncounted, so elapsed + // time is the real limit and Check 2's count is the backstop behind it. if (_config.RateLimitConfig.Enabled && clearedState.RateLimitStartTime.HasValue && currentTime - clearedState.RateLimitStartTime.Value diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs index a57d506..1696427 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateStorage.cs @@ -52,8 +52,14 @@ private static JsonObject Serialize(RetryState state) }; if (state.WaitUntilTime.HasValue) root["waitUntilTime"] = state.WaitUntilTime.Value; - if (state.RateLimitStartTime.HasValue) - root["rateLimitStartTime"] = state.RateLimitStartTime.Value; + + // RateLimitStartTime is deliberately not persisted. It measures how long this + // process has been retrying a batch, and time while the process was not + // running is not time spent retrying. Persisting it means an app closed for + // longer than MaxRateLimitDuration loads an already-expired episode and + // discards the batch on its first flush without ever having retried it. The + // retry counts below do persist, so a batch still cannot be retried + // indefinitely across restarts. if (state.BatchMetadata.Count > 0) { @@ -85,7 +91,6 @@ 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(); @@ -104,7 +109,8 @@ private static RetryState Deserialize(JsonObject root) } } - return new RetryState(pipelineState, waitUntilTime, globalRetryCount, batchMetadata, rateLimitStartTime); + // rateLimitStartTime is intentionally absent; see Serialize. + return new RetryState(pipelineState, waitUntilTime, globalRetryCount, batchMetadata); } private static int ReadInt(JsonObject json, string key) diff --git a/Tests/Retry/RetryStateStorageTest.cs b/Tests/Retry/RetryStateStorageTest.cs index dfb89c9..0408065 100644 --- a/Tests/Retry/RetryStateStorageTest.cs +++ b/Tests/Retry/RetryStateStorageTest.cs @@ -52,10 +52,12 @@ public void RoundTrip_RateLimitedState() } [Fact] - public void RoundTrip_RateLimitStartTime() + public void RateLimitStartTimeIsNotCarriedAcrossRestarts() { - // MaxRateLimitDuration measures from this, so losing it across a restart - // would restart the episode clock and let a batch outlive its budget. + // It measures how long this process has been retrying. Carrying it over + // means an app closed for longer than MaxRateLimitDuration loads an + // already-expired episode and discards the batch on its first flush, + // having never retried it while running. The counts do carry over. var state = new RetryState( pipelineState: PipelineState.RateLimited, waitUntilTime: 1_700_000_030_000, @@ -65,7 +67,7 @@ public void RoundTrip_RateLimitStartTime() RetryStateStorage.SaveRetryState(_storage.Object, state); RetryState loaded = RetryStateStorage.LoadRetryState(_storage.Object); - Assert.Equal(1_700_000_000_000, loaded.RateLimitStartTime); + Assert.Null(loaded.RateLimitStartTime); Assert.Equal(1_700_000_030_000, loaded.WaitUntilTime); Assert.Equal(3, loaded.GlobalRetryCount); } From 62ea228237aa21c94741d41412ba2f6f9896b93e Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 19:50:58 -0400 Subject: [PATCH 4/9] Test the rate-limit wait clamp, which nothing exercised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clamp in HandleRateLimitResponse had no coverage at all. Deleting it outright left all 264 tests passing: the two test files this change touched check config validation and arithmetic on default constants, and neither calls the state machine. The one part of the change with any logic in it was the one part unguarded. Two tests now drive HandleResponse directly through a fake clock. The first opens an episode, advances to one second before the budget ends, sends a 429 asking for sixty, and asserts the wait lands on the episode deadline — it fails with the clamp removed. The second asserts a wait that comfortably fits is passed through untouched, so the clamp cannot degenerate into truncating every Retry-After to the deadline. 266 unit tests and the 82-test e2e suite pass. --- Tests/Retry/RetryStateMachineTest.cs | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/Tests/Retry/RetryStateMachineTest.cs b/Tests/Retry/RetryStateMachineTest.cs index 9d8df50..8ce2b83 100644 --- a/Tests/Retry/RetryStateMachineTest.cs +++ b/Tests/Retry/RetryStateMachineTest.cs @@ -486,6 +486,48 @@ public void GetRetryCount_GlobalHigher_ReturnsGlobal() Assert.Equal(10, machine.GetRetryCount(state, "batch1.json")); } + [Fact] + public void RateLimitWaitIsClampedToTheEndOfTheBudget() + { + // The only behavioural test of the clamp itself. Deleting the clamp from + // HandleRateLimitResponse leaves every other test in the suite passing, + // because the rest exercise config validation and arithmetic on defaults + // rather than the state machine. + var clock = new FakeTimeProvider(); + var machine = CreateMachine( + maxRetryCount: 1000, timeProvider: clock, maxRateLimitDuration: 300); + + long began = clock.CurrentTimeMillis(); + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 5, "b.json", began)); + + // 1 second of budget left, and the server asks for 60. + clock.Time += 299_000; + state = machine.HandleResponse( + state, new ResponseInfo(429, 60, "b.json", clock.CurrentTimeMillis())); + + long deadline = began + (300 * 1000L); + Assert.Equal(deadline, state.WaitUntilTime); + Assert.True( + state.WaitUntilTime <= deadline, + $"waited until {state.WaitUntilTime}, past the episode deadline {deadline}"); + } + + [Fact] + public void RateLimitWaitIsUnclampedWhileTheBudgetIsAmple() + { + // The clamp must not shorten a wait that fits, or every Retry-After would + // be truncated to the episode deadline rather than honoured. + var clock = new FakeTimeProvider(); + var machine = CreateMachine(timeProvider: clock, maxRateLimitDuration: 300); + + long now = clock.CurrentTimeMillis(); + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 30, "b.json", now)); + + Assert.Equal(now + 30_000, state.WaitUntilTime); + } + [Fact] public void RateLimitEpisodeIsBoundedByMaxRateLimitDuration() { From b6c0d404230ef2bd0f7d4cd86684e68962d8d293 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 21:19:06 -0400 Subject: [PATCH 5/9] Cut the comments back to why, not history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the team convention to my own work from today. The comments explaining these changes had accumulated into potted histories: why a value had been twelve hours, what a test used to assert, which path used to be unreachable. Six months from now none of that resolves to anything — the diff and the commit messages hold it, and the comment should say why the code is the way it is. What stayed is what a maintainer would undo without it: that Kernel#sleep raises on a negative interval, that Thread#wakeup only interrupts a sleep already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an interrupt does not reach them, and that inverting one assertion would make the duration budget unreachable again. Comments only, no behaviour change. --- Tests/Retry/ConfigurationHttpConfigTest.cs | 9 ++++----- Tests/Retry/RetryStateMachineTest.cs | 11 +++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index efca52f..cc0f2b8 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -152,11 +152,10 @@ public void MaxTotalBackoffDurationOfZeroDoesNotAbandonOnTheSecondAttempt() [Fact] public void MaxRateLimitDurationIsTheOperativeLimitNotTheCount() { - // This assertion is the inverse of what it was, deliberately. The count used - // to be the working limit with a 12h duration as an unreachable backstop — - // but rate-limited attempts are uncounted across the other SDKs, so the - // duration is what genuinely bounds this path. At 5 minutes against a 60s - // ceiling it now trips first, and the count is the backstop. + // Deliberately this way round, and it reads backwards at a glance. Rate- + // limited attempts are uncounted, so the duration is what genuinely bounds + // this path; the count sits behind it as a backstop. Inverting this to + // "the count trips first" would make the duration unreachable again. var rateLimit = new RateLimitConfig(); long countWouldAllowSeconds = diff --git a/Tests/Retry/RetryStateMachineTest.cs b/Tests/Retry/RetryStateMachineTest.cs index 8ce2b83..adf8746 100644 --- a/Tests/Retry/RetryStateMachineTest.cs +++ b/Tests/Retry/RetryStateMachineTest.cs @@ -489,10 +489,9 @@ public void GetRetryCount_GlobalHigher_ReturnsGlobal() [Fact] public void RateLimitWaitIsClampedToTheEndOfTheBudget() { - // The only behavioural test of the clamp itself. Deleting the clamp from - // HandleRateLimitResponse leaves every other test in the suite passing, - // because the rest exercise config validation and arithmetic on defaults - // rather than the state machine. + // Guards the clamp in HandleRateLimitResponse. The other retry tests check + // config validation and arithmetic on defaults, so they stay green whether + // the clamp is there or not. var clock = new FakeTimeProvider(); var machine = CreateMachine( maxRetryCount: 1000, timeProvider: clock, maxRateLimitDuration: 300); @@ -531,8 +530,8 @@ public void RateLimitWaitIsUnclampedWhileTheBudgetIsAmple() [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. + // The wall-clock bound on one episode. Without it a pathological + // Retry-After stream is limited only by the retry count. var clock = new FakeTimeProvider(); var machine = CreateMachine( maxRetryCount: 1000, timeProvider: clock, maxRateLimitDuration: 60); From ea238fdcaea8423769a5bcc21afbd2f3368f7516 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 24 Sep 2026 09:54:14 -0400 Subject: [PATCH 6/9] Honour Retry-After up to 300s rather than capping it at 60 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capping at 60s meant waiting less than the server asked for, which does not make the next attempt more likely to succeed — it just sends more requests at something already rate-limiting us. Against a Retry-After of 180s inside a 5 minute budget it turns 3 requests into 6; against 300s it turns 2 into 6. The cap is a guard against an absurd header, not a second budget. How long we keep trying is max_rate_limit_duration's job, and the clamp to the remaining budget already stops a single wait running past it, so the cap now rarely binds at all. It also bought nothing for the client this was partly aimed at: with no background thread, a shorter cap turns one long wait into several short ones for the same total blocking time and more requests. Tests that pinned 60 are updated, and each SDK gains one asserting that a Retry-After inside the cap is used as given rather than shortened. --- .../Segment/Analytics/Retry/RetryConfig.cs | 10 ++++++---- CHANGELOG.md | 2 +- Tests/Retry/ConfigurationHttpConfigTest.cs | 18 +++++++++--------- Tests/Retry/HttpConfigParserTest.cs | 7 +++---- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 313b87c..e9f6b40 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -5,10 +5,12 @@ namespace Segment.Analytics.Retry { public class RateLimitConfig { - /// Largest Retry-After the client will honour, in seconds. Kept well below - /// so the budget buys several attempts rather than - /// one long sleep; at the old 300s a single sleep consumed the whole budget. - public const int MaxRetryIntervalCeiling = 60; + /// Largest Retry-After the client will honour, in seconds. A guard against + /// an absurd header, not a second budget: waiting less than the server asked for does + /// not make the next attempt more likely to succeed, it just sends more requests at + /// something already rate-limiting us. How long we keep trying is + /// 's job. + public const int MaxRetryIntervalCeiling = 300; public bool Enabled { get; } public int MaxRetryCount { get; } diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ab4aaa..85bd45b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ proxy they point at. ### Retry handling -- A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `RateLimitConfig.MaxRetryInterval` (default 60 seconds). +- A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `RateLimitConfig.MaxRetryInterval` (default 300 seconds). - Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 5 minutes). Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. - 511 is dropped rather than retried: it asks the client to re-authenticate, which this library cannot do. - `RetryBehavior`, `RateLimitConfig`, `BackoffConfig` and `HttpConfig` are now public, and `HttpConfig` is a settable property on `Configuration`, so retry behaviour can be configured in code. On mobile targets, CDN settings replace it when present. diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index cc0f2b8..48e7548 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -168,17 +168,17 @@ public void MaxRateLimitDurationIsTheOperativeLimitNotTheCount() } [Fact] - public void RetryAfterIsCappedWellBelowTheBudget() + public void RetryAfterCeilingClampsAnAbsurdValue() { - // A cap equal to the budget would let one sleep consume it, leaving the - // rate-limit path with a single attempt. - var validated = new RateLimitConfig(maxRetryInterval: 3600).Validated(); + // The ceiling exists to reject a nonsense header, not to shorten a + // reasonable one — a value inside it is honoured as given, because waiting + // less than asked only adds requests against a server already rate-limiting + // us. MaxRateLimitDuration is what bounds how long we keep trying. + Assert.Equal( + RateLimitConfig.MaxRetryIntervalCeiling, + new RateLimitConfig(maxRetryInterval: 3600).Validated().MaxRetryInterval); - Assert.Equal(RateLimitConfig.MaxRetryIntervalCeiling, validated.MaxRetryInterval); - Assert.Equal(60, validated.MaxRetryInterval); - Assert.True( - validated.MaxRetryInterval * 4 <= validated.MaxRateLimitDuration, - "the budget should buy at least a few attempts, not one"); + Assert.Equal(120, new RateLimitConfig(maxRetryInterval: 120).Validated().MaxRetryInterval); } [Fact] diff --git a/Tests/Retry/HttpConfigParserTest.cs b/Tests/Retry/HttpConfigParserTest.cs index 5abb54b..0c2cc0b 100644 --- a/Tests/Retry/HttpConfigParserTest.cs +++ b/Tests/Retry/HttpConfigParserTest.cs @@ -54,7 +54,7 @@ public void Parse_RateLimitConfig_ParsesValues() HttpConfig config = HttpConfigParser.Parse(json); Assert.Equal(10, config.RateLimitConfig.MaxRetryCount); - // Under the 60s ceiling, so it survives validation unchanged. + // Under the ceiling, so it survives validation unchanged. Assert.Equal(45, config.RateLimitConfig.MaxRetryInterval); } @@ -96,9 +96,8 @@ public void Parse_ClampsValues() HttpConfig config = HttpConfigParser.Parse(json); Assert.Equal(1000, config.RateLimitConfig.MaxRetryCount); - // Retry-After is capped at 60s: well below the 5 minute rate-limit budget, so - // the budget buys several attempts rather than one long sleep. - Assert.Equal(60, config.RateLimitConfig.MaxRetryInterval); + // Clamped to the Retry-After ceiling. + Assert.Equal(300, config.RateLimitConfig.MaxRetryInterval); Assert.Equal(60.0, config.BackoffConfig.BaseBackoffInterval); Assert.Equal(3600, config.BackoffConfig.MaxBackoffInterval); } From b570caf73217549de63bb3672b59db57e181c504 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 14:33:20 -0400 Subject: [PATCH 7/9] Raise the rate-limit budget to 30 minutes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The budget and the Retry-After cap were both 300s, and at parity the rate-limit path degenerates. A response with no usable Retry-After waits the cap by default, the elapsed check runs before the wait, so that one wait spends the whole budget and the batch is dropped having been tried once. A legitimate Retry-After of 300 does the same. The cap also stops binding: whatever is left of the budget is always the smaller term, so the cap can never be the value that clamps. Thirty minutes restores the relationship the two knobs are meant to have — the cap bounds one wait, the budget bounds the episode — and leaves room for several attempts. It costs nothing in normal operation, since the budget only binds when the server has been rate-limiting us for a long time, and in that case keeping the data is the point. --- .../Segment/Analytics/Retry/RetryConfig.cs | 11 +++++++--- CHANGELOG.md | 2 +- Tests/Retry/ConfigurationHttpConfigTest.cs | 20 +++++++++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index e9f6b40..52ddd35 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -18,9 +18,14 @@ public class RateLimitConfig /// /// Wall-clock ceiling, in seconds, on how long one rate-limit episode may keep a - /// batch alive. Five minutes, in line with the counted-backoff path's ~4 minute - /// worst case. This is the operative limit on that path: rate-limited attempts are + /// batch alive. This is the operative limit on that path: rate-limited attempts are /// deliberately uncounted, so a duration is the only thing bounding them. + /// + /// Deliberately several times . When the two are + /// equal, a response with no usable Retry-After waits + /// by default, which consumes the entire budget — the elapsed check runs before the + /// wait, so the batch is dropped after a single attempt having stalled the whole + /// pipeline for the duration. /// public long MaxRateLimitDuration { get; } @@ -28,7 +33,7 @@ public RateLimitConfig( bool enabled = true, int maxRetryCount = 100, int maxRetryInterval = MaxRetryIntervalCeiling, - long maxRateLimitDuration = 300) + long maxRateLimitDuration = 1800) { Enabled = enabled; MaxRetryCount = maxRetryCount; diff --git a/CHANGELOG.md b/CHANGELOG.md index 85bd45b..5d3e790 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ proxy they point at. ### Retry handling - A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `RateLimitConfig.MaxRetryInterval` (default 300 seconds). -- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 5 minutes). Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. +- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 30 minutes). Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. - 511 is dropped rather than retried: it asks the client to re-authenticate, which this library cannot do. - `RetryBehavior`, `RateLimitConfig`, `BackoffConfig` and `HttpConfig` are now public, and `HttpConfig` is a settable property on `Configuration`, so retry behaviour can be configured in code. On mobile targets, CDN settings replace it when present. - `BackoffConfig.StatusCodeOverrides` is merged over the built-in defaults rather than replacing them, so overriding one status leaves the rest unchanged. diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 48e7548..0a6c2bd 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -167,6 +167,26 @@ public void MaxRateLimitDurationIsTheOperativeLimitNotTheCount() + $"{countWouldAllowSeconds}s; the duration should bound this path"); } + [Fact] + public void DefaultBudgetLeavesRoomForMoreThanOneMaximalWait() + { + // At parity the rate-limit path performs no retries at all. A response with + // no usable Retry-After waits MaxRetryInterval by default, ShouldUploadBatch + // tests elapsed time before the wait, so that one wait spends the budget and + // the next evaluation drops the batch — one attempt, having stalled the whole + // pipeline for the duration first. + var rateLimit = new RateLimitConfig(); + + Assert.True( + rateLimit.MaxRateLimitDuration > rateLimit.MaxRetryInterval, + $"budget is {rateLimit.MaxRateLimitDuration}s against a {rateLimit.MaxRetryInterval}s " + + "interval; a single maximal wait would consume it and leave no retry"); + + Assert.True( + rateLimit.MaxRateLimitDuration / rateLimit.MaxRetryInterval >= 2, + "budget leaves room for fewer than two maximal waits"); + } + [Fact] public void RetryAfterCeilingClampsAnAbsurdValue() { From e2bafb62cc0c8d0b24fe2be635622ce9dcbf6bc8 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 16:07:58 -0400 Subject: [PATCH 8/9] Document the rate-limit retry count RateLimitConfig.MaxRetryCount bounds the rate-limit path alongside the duration, and it appeared in no changelog in either SDK generation: the rewrite dropped the old "MaxRetryCount 10 (was 100)" line, and the new entry names only BackoffConfig.MaxRetryCount. This is the one SDK where a rate-limited retry consumes a count at all, so a reader comparing behaviour had no way to find the number. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d3e790..402ad77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ proxy they point at. ### Retry handling - A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `RateLimitConfig.MaxRetryInterval` (default 300 seconds). -- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 30 minutes). Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. +- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 30 minutes), or `RateLimitConfig.MaxRetryCount` attempts (default 100), whichever comes first. The duration is the operative limit at the defaults; the count is a backstop against a server repeating a very short `Retry-After`. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. - 511 is dropped rather than retried: it asks the client to re-authenticate, which this library cannot do. - `RetryBehavior`, `RateLimitConfig`, `BackoffConfig` and `HttpConfig` are now public, and `HttpConfig` is a settable property on `Configuration`, so retry behaviour can be configured in code. On mobile targets, CDN settings replace it when present. - `BackoffConfig.StatusCodeOverrides` is merged over the built-in defaults rather than replacing them, so overriding one status leaves the rest unchanged. From 2946a7b6eddcf4ad27cb4415fe293f6285b2a86c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 17:05:09 -0400 Subject: [PATCH 9/9] End an episode rather than resume inside the window, and stop stranding its clock Four review points. The remaining-budget clamp is replaced by a drop. Shortening a Retry-After to fit the budget resumes inside the window the server named -- one it has already said it will not serve -- and ShouldUploadBatch drops the batch on the elapsed check immediately afterwards, so the shortened wait bought exactly one refused request. A non-retryable response no longer strands the episode clock. RateLimitStartTime was cleared on success and on the two budget-exceeded paths but not when a 4xx ended the batch, and nothing else clears it, so the next batch evaluated after the budget elapsed was dropped for a rate limit that had already ended, without ever being uploaded. Same defect python had; this SDK was wrongly cleared of it. The rate-limit path is bounded by a count as well as a duration, and the comment and changelog claimed the duration was the operative limit. It is not: the crossover sits at MaxRateLimitDuration / MaxRetryCount, 18 seconds at the defaults, so the count bounds every episode with a shorter Retry-After, which is most of them. Raising the budget to 30 minutes moved that crossover from 3s to 18s and made the claim materially wrong rather than marginally so. The test that was supposed to guard this asserted the duration was below the count's theoretical maximum span, 100 x 300s. True, and it passes while the count is the limit actually reached. It now asserts the crossover from both sides and pins the number. Validated() floored maxRateLimitDuration at 0 while its two siblings floor at 1 with comments explaining why. A CDN payload pushing 0 disabled rate-limit retrying entirely. 269 tests pass. --- .../Segment/Analytics/Retry/RetryConfig.cs | 14 ++++- .../Analytics/Retry/RetryStateMachine.cs | 26 +++++++-- CHANGELOG.md | 2 +- Tests/Retry/ConfigurationHttpConfigTest.cs | 30 ++++++---- Tests/Retry/RetryStateMachineTest.cs | 55 ++++++++++++++++--- 5 files changed, 99 insertions(+), 28 deletions(-) diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs index 52ddd35..0f75336 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs @@ -18,8 +18,13 @@ public class RateLimitConfig /// /// Wall-clock ceiling, in seconds, on how long one rate-limit episode may keep a - /// batch alive. This is the operative limit on that path: rate-limited attempts are - /// deliberately uncounted, so a duration is the only thing bounding them. + /// batch alive. + /// + /// Unlike the other Segment SDKs, this one bounds the rate-limit path by a + /// count as well (), and which of the two binds depends + /// on the Retry-After being served: below roughly + /// MaxRateLimitDuration / MaxRetryCount — 18 seconds at the defaults — the + /// count runs out first, above it the duration does. /// /// Deliberately several times . When the two are /// equal, a response with no usable Retry-After waits @@ -47,7 +52,10 @@ public RateLimitConfig( // 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)) + // Floored at 1 like its siblings: 0 here means "no budget", so every + // rate-limited batch is dropped on its first evaluation. A CDN payload + // pushing 0 would silently disable rate-limit retrying altogether. + maxRateLimitDuration: Math.Max(1, Math.Min(MaxRateLimitDuration, 604800)) ); } diff --git a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs index 05bdcac..e248c08 100644 --- a/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs +++ b/Analytics-CSharp/Segment/Analytics/Retry/RetryStateMachine.cs @@ -66,7 +66,13 @@ public RetryState HandleResponse(RetryState state, ResponseInfo response) if (statusBehavior == RetryBehavior.Retry && _config.BackoffConfig.Enabled) return HandleRetryableError(state, response, currentTime); - return state.RemoveBatch(response.BatchFile); + // The request completed and carried no rate-limit signal, so the episode is + // over. Leaving RateLimitStartTime set strands it: nothing else clears it, + // and the next batch to be evaluated after the budget elapses is dropped for + // a rate limit that ended here, without ever being uploaded. + return state + .With(clearRateLimitStartTime: true) + .RemoveBatch(response.BatchFile); } public Tuple ShouldUploadBatch(RetryState state, string batchFile) @@ -200,13 +206,23 @@ private RetryState HandleRateLimitResponse(RetryState state, ResponseInfo respon { long waitUntilTimeMs = CalculateWaitUntilTimeMs(response.RetryAfterSeconds, currentTime); - // Clamped to the end of the budget: ShouldUploadBatch checks elapsed time - // before the wait, so without this a check passing just inside the budget - // would wait a full Retry-After beyond it. + // A wait that runs past the end of the budget ends the episode. Shortening + // it to fit would resume inside the window the server asked us to wait out + // -- one it has already said it will not serve -- and ShouldUploadBatch + // would then drop the batch on the elapsed check anyway, so the shortened + // wait buys a single guaranteed-refused request. long episodeStart = state.RateLimitStartTime ?? currentTime; long deadline = episodeStart + (_config.RateLimitConfig.MaxRateLimitDuration * 1000L); if (waitUntilTimeMs > deadline) - waitUntilTimeMs = deadline; + { + return state + .With( + pipelineState: PipelineState.Ready, + clearWaitUntilTime: true, + globalRetryCount: 0, + clearRateLimitStartTime: true) + .RemoveBatch(response.BatchFile); + } return state.With( pipelineState: PipelineState.RateLimited, waitUntilTime: waitUntilTimeMs, diff --git a/CHANGELOG.md b/CHANGELOG.md index 402ad77..07984ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ proxy they point at. ### Retry handling - A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `RateLimitConfig.MaxRetryInterval` (default 300 seconds). -- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 30 minutes), or `RateLimitConfig.MaxRetryCount` attempts (default 100), whichever comes first. The duration is the operative limit at the defaults; the count is a backstop against a server repeating a very short `Retry-After`. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. +- Responses carrying `Retry-After` are retried for up to `RateLimitConfig.MaxRateLimitDuration` (default 30 minutes), or `RateLimitConfig.MaxRetryCount` attempts (default 100), whichever comes first. Which one binds depends on the interval being served: below roughly `MaxRateLimitDuration / MaxRetryCount` — 18 seconds at the defaults — the count runs out first, above it the duration does. A `Retry-After` that will not fit in what is left of the budget ends the episode rather than being shortened, since resuming inside the window the server named sends a request it has already declined to serve. Other failures use exponential backoff from 500ms to a 60 second ceiling, limited by `BackoffConfig.MaxRetryCount` (default 10) and by `BackoffConfig.MaxTotalBackoffDuration` (default 12 hours) as an upper bound. - 511 is dropped rather than retried: it asks the client to re-authenticate, which this library cannot do. - `RetryBehavior`, `RateLimitConfig`, `BackoffConfig` and `HttpConfig` are now public, and `HttpConfig` is a settable property on `Configuration`, so retry behaviour can be configured in code. On mobile targets, CDN settings replace it when present. - `BackoffConfig.StatusCodeOverrides` is merged over the built-in defaults rather than replacing them, so overriding one status leaves the rest unchanged. diff --git a/Tests/Retry/ConfigurationHttpConfigTest.cs b/Tests/Retry/ConfigurationHttpConfigTest.cs index 0a6c2bd..e47dddd 100644 --- a/Tests/Retry/ConfigurationHttpConfigTest.cs +++ b/Tests/Retry/ConfigurationHttpConfigTest.cs @@ -150,21 +150,31 @@ public void MaxTotalBackoffDurationOfZeroDoesNotAbandonOnTheSecondAttempt() } [Fact] - public void MaxRateLimitDurationIsTheOperativeLimitNotTheCount() + public void WhichRateLimitBoundBindsDependsOnTheRetryAfterBeingServed() { - // Deliberately this way round, and it reads backwards at a glance. Rate- - // limited attempts are uncounted, so the duration is what genuinely bounds - // this path; the count sits behind it as a backstop. Inverting this to - // "the count trips first" would make the duration unreachable again. + // The previous version of this test asserted the duration was below the + // count's theoretical maximum span (100 x 300s), which is true and says + // nothing: it passes while the count is the limit actually reached. The + // crossover is what matters, and at the defaults it sits at 18 seconds -- + // so the count, not the duration, bounds every episode whose Retry-After + // is shorter than that, which is most of them. var rateLimit = new RateLimitConfig(); - long countWouldAllowSeconds = - (long)rateLimit.MaxRetryCount * RateLimitConfig.MaxRetryIntervalCeiling; + double crossoverSeconds = + (double)rateLimit.MaxRateLimitDuration / rateLimit.MaxRetryCount; + // Below the crossover the count runs out first. Assert.True( - rateLimit.MaxRateLimitDuration < countWouldAllowSeconds, - $"duration is {rateLimit.MaxRateLimitDuration}s but the count would allow " - + $"{countWouldAllowSeconds}s; the duration should bound this path"); + rateLimit.MaxRetryCount * (crossoverSeconds / 2) < rateLimit.MaxRateLimitDuration, + "expected the count to bind for a Retry-After below the crossover"); + + // Above it the duration does. + Assert.True( + rateLimit.MaxRetryCount * (crossoverSeconds * 2) > rateLimit.MaxRateLimitDuration, + "expected the duration to bind for a Retry-After above the crossover"); + + // Documented so a change to either constant has to restate it. + Assert.Equal(18.0, crossoverSeconds); } [Fact] diff --git a/Tests/Retry/RetryStateMachineTest.cs b/Tests/Retry/RetryStateMachineTest.cs index adf8746..585ff44 100644 --- a/Tests/Retry/RetryStateMachineTest.cs +++ b/Tests/Retry/RetryStateMachineTest.cs @@ -487,11 +487,12 @@ public void GetRetryCount_GlobalHigher_ReturnsGlobal() } [Fact] - public void RateLimitWaitIsClampedToTheEndOfTheBudget() + public void ARateLimitWaitThatCannotFitTheBudgetEndsTheEpisode() { - // Guards the clamp in HandleRateLimitResponse. The other retry tests check - // config validation and arithmetic on defaults, so they stay green whether - // the clamp is there or not. + // Shortening it to fit would resume inside the window the server named -- + // one it has already said it will not serve -- and ShouldUploadBatch would + // drop the batch on the elapsed check straight afterwards, so the shortened + // wait buys exactly one refused request. var clock = new FakeTimeProvider(); var machine = CreateMachine( maxRetryCount: 1000, timeProvider: clock, maxRateLimitDuration: 300); @@ -505,11 +506,47 @@ public void RateLimitWaitIsClampedToTheEndOfTheBudget() state = machine.HandleResponse( state, new ResponseInfo(429, 60, "b.json", clock.CurrentTimeMillis())); - long deadline = began + (300 * 1000L); - Assert.Equal(deadline, state.WaitUntilTime); - Assert.True( - state.WaitUntilTime <= deadline, - $"waited until {state.WaitUntilTime}, past the episode deadline {deadline}"); + Assert.Null(state.WaitUntilTime); + Assert.Null(state.RateLimitStartTime); + Assert.Equal(PipelineState.Ready, state.PipelineState); + Assert.False(state.BatchMetadata.ContainsKey("b.json")); + } + + [Fact] + public void ARateLimitWaitThatFitsIsHonouredInFull() + { + // "Never shorten" must not become "never wait". + var clock = new FakeTimeProvider(); + var machine = CreateMachine( + maxRetryCount: 1000, timeProvider: clock, maxRateLimitDuration: 300); + + long began = clock.CurrentTimeMillis(); + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 60, "b.json", began)); + + Assert.Equal(began + 60_000, state.WaitUntilTime); + Assert.Equal(PipelineState.RateLimited, state.PipelineState); + } + + [Fact] + public void ANonRetryableResponseEndsTheRateLimitEpisode() + { + // The request completed and carried no rate-limit signal. Leaving + // RateLimitStartTime set strands it -- nothing else clears it -- and the + // next batch evaluated after the budget elapses is dropped for a rate limit + // that ended here, without ever being uploaded. + var clock = new FakeTimeProvider(); + var machine = CreateMachine(timeProvider: clock, maxRateLimitDuration: 300); + + RetryState state = machine.HandleResponse( + new RetryState(), new ResponseInfo(429, 5, "b.json", clock.CurrentTimeMillis())); + Assert.NotNull(state.RateLimitStartTime); + + clock.Time += 1_000; + state = machine.HandleResponse( + state, new ResponseInfo(400, null, "b.json", clock.CurrentTimeMillis())); + + Assert.Null(state.RateLimitStartTime); } [Fact]