Unified HTTP response handling and retry behavior, enabled by default

Analytics-CSharp/Segment/Analytics/Retry/RetryConfig.cs

@@ -5,8 +5,11 @@ namespace Segment.Analytics.Retry
 {
     public class RateLimitConfig
     {
-        /// <summary>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.</summary>
+        /// <summary>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
+        /// <see cref="MaxRateLimitDuration"/>'s job.</summary>
         public const int MaxRetryIntervalCeiling = 300;
 
         public bool Enabled { get; }
@@ -15,18 +18,27 @@ public class RateLimitConfig
 
         /// <summary>
         /// 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; <see cref="MaxRetryCount"/> 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.
+        ///
+        /// <para>Unlike the other Segment SDKs, this one bounds the rate-limit path by a
+        /// count as well (<see cref="MaxRetryCount"/>), and which of the two binds depends
+        /// on the Retry-After being served: below roughly
+        /// <c>MaxRateLimitDuration / MaxRetryCount</c> — 18 seconds at the defaults — the
+        /// count runs out first, above it the duration does.</para>
+        ///
+        /// <para>Deliberately several times <see cref="MaxRetryInterval"/>. When the two are
+        /// equal, a response with no usable Retry-After waits <see cref="MaxRetryInterval"/>
+        /// 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.</para>
         /// </summary>
         public long MaxRateLimitDuration { get; }
 
         public RateLimitConfig(
             bool enabled = true,
             int maxRetryCount = 100,
-            int maxRetryInterval = 300,
-            long maxRateLimitDuration = 43200)
+            int maxRetryInterval = MaxRetryIntervalCeiling,
+            long maxRateLimitDuration = 1800)
         {
             Enabled = enabled;
             MaxRetryCount = maxRetryCount;
@@ -40,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))
         );
     }
 

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<UploadDecision, RetryState> ShouldUploadBatch(RetryState state, string batchFile)
@@ -104,9 +110,9 @@ public Tuple<UploadDecision, RetryState> 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
@@ -199,6 +205,24 @@ public bool ShouldDeleteBatch(int statusCode, int? retryAfterSeconds)
         private RetryState HandleRateLimitResponse(RetryState state, ResponseInfo response, long currentTime)
         {
             long waitUntilTimeMs = CalculateWaitUntilTimeMs(response.RetryAfterSeconds, currentTime);
+
+            // 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)
+            {
+                return state
+                    .With(
+                        pipelineState: PipelineState.Ready,
+                        clearWaitUntilTime: true,
+                        globalRetryCount: 0,
+                        clearRateLimitStartTime: true)
+                    .RemoveBatch(response.BatchFile);
+            }
             return state.With(
                 pipelineState: PipelineState.RateLimited,
                 waitUntilTime: waitUntilTimeMs,

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<string, BatchMetadata>();
@@ -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)

CHANGELOG.md

@@ -6,16 +6,16 @@ This file carries the notes that need more than a pull-request title.
 
 ## Unreleased
 
-### Behavior change: retries and backoff are on by default
+### Behaviour change: retries and backoff are on by default
 
-Through 2.6.0, rate limiting and exponential backoff were both disabled unless you
-supplied an `HttpConfig` or a CDN settings payload turned them on. Server-side
-deployments receive no CDN settings, so in practice they retried nothing: 408, 410 and
-460 were dropped, `Retry-After` was ignored, and a 429 or 5xx was held with no delay and
-no budget. Both subsystems now default to enabled, so a client that configures nothing
-gets the documented retry behavior.
+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.
 
-To keep the old behavior, disable both explicitly:
+To keep the previous behaviour, disable both explicitly:
 
 ```csharp
 new Configuration("writeKey")
@@ -26,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.
+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.
 
-- Backoff defaults now match the other Segment SDKs: `MaxRetryCount` 10 (was 100) and `MaxBackoffInterval` 60s (was 300s). With retries off by default those numbers were latent; enabling them unchanged would have had C# clients making an order of magnitude more attempts against the endpoint than any other SDK.
-
-### Upgrade note: new request headers and proxy allowlists
+### 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 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. 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.
+- 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.