feat(core): honor Retry-After on any retryable code (HTTP 529 + beyond) (#1294)

Per the "Generic Retry-After Header Support" design (Option B), the
Retry-After header — not the specific status code — is the authoritative
signal to back off. Previously the SDK only parsed Retry-After on 429 and
let every other retryable code (529, 503, 408, …) fall into exponential
backoff, ignoring the server's requested wait.

SegmentDestination now parses Retry-After on any error response and routes
retryable responses that carry it through the server-directed wait path,
while codes without the header keep exponential backoff and permanent codes
still drop. RetryManager gains handleRetryAfter() as the generic entry point;
handle429() delegates to it, preserving existing 429 behavior.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: abueide

e2e-cli/e2e-config.json

@@ -5,6 +5,7 @@
   "patch": null,
   "env": {
     "BROWSER_BATCHING": "true",
-    "HTTP_CONFIG_SETTINGS": "true"
+    "HTTP_CONFIG_SETTINGS": "true",
+    "GENERIC_RETRY_AFTER": "true"
   }
 }

packages/core/src/backoff/RetryManager.ts

@@ -164,10 +164,18 @@ export class RetryManager {
   }
 
   /**
-   * Handle a 429 rate limit response.
-   * Uses server-specified wait time from Retry-After header.
+   * Handle a response that carries a server-directed wait via the Retry-After
+   * header. Originally added for 429 rate limiting, this is the authoritative
+   * "the server told us exactly how long to wait" path and is reused for any
+   * retryable status code (429, 529, 503, 408, …) that includes Retry-After.
+   *
+   * Uses the server-specified wait time (clamped to maxRetryInterval), pauses
+   * all uploads, and enforces the same maxRetryCount / maxRateLimitDuration
+   * safety valves as the 429 path.
    */
-  async handle429(retryAfterSeconds: number): Promise<RetryResult | undefined> {
+  async handleRetryAfter(
+    retryAfterSeconds: number
+  ): Promise<RetryResult | undefined> {
     if (this.rateLimitConfig?.enabled !== true) {
       return undefined;
     }
@@ -189,6 +197,14 @@ export class RetryManager {
     );
   }
 
+  /**
+   * Handle a 429 rate limit response.
+   * Delegates to the shared Retry-After (server-directed wait) path.
+   */
+  async handle429(retryAfterSeconds: number): Promise<RetryResult | undefined> {
+    return this.handleRetryAfter(retryAfterSeconds);
+  }
+
   /**
    * Handle a transient error (5xx, network failure).
    * Uses exponential backoff to calculate wait time.

packages/core/src/backoff/__tests__/RetryManager.test.ts

@@ -146,6 +146,75 @@ describe('RetryManager', () => {
     });
   });
 
+  describe('handleRetryAfter (generic server-directed wait)', () => {
+    it('waits the server-directed time for a non-429 code', async () => {
+      const now = 1000000;
+      jest.spyOn(Date, 'now').mockReturnValue(now);
+
+      const rm = new RetryManager(
+        'test-key',
+        mockPersistor,
+        defaultRateLimitConfig,
+        defaultBackoffConfig,
+        mockLogger
+      );
+
+      // e.g. a 529 with Retry-After: 30
+      expect(await rm.handleRetryAfter(30)).toBe('rate_limited');
+
+      jest.spyOn(Date, 'now').mockReturnValue(now + 29000);
+      expect(await rm.canRetry()).toBe(false);
+      jest.spyOn(Date, 'now').mockReturnValue(now + 31000);
+      expect(await rm.canRetry()).toBe(true);
+    });
+
+    it('clamps the wait to maxRetryInterval', async () => {
+      const rm = new RetryManager(
+        'test-key',
+        mockPersistor,
+        defaultRateLimitConfig,
+        defaultBackoffConfig,
+        mockLogger
+      );
+
+      await rm.handleRetryAfter(500); // exceeds maxRetryInterval of 300
+
+      expect(mockLogger.warn).toHaveBeenCalledWith(
+        expect.stringContaining('exceeds maxRetryInterval')
+      );
+    });
+
+    it('returns undefined when rate limit config is disabled', async () => {
+      const disabledConfig: RateLimitConfig = {
+        ...defaultRateLimitConfig,
+        enabled: false,
+      };
+      const rm = new RetryManager(
+        'test-key',
+        mockPersistor,
+        disabledConfig,
+        defaultBackoffConfig,
+        mockLogger
+      );
+
+      expect(await rm.handleRetryAfter(30)).toBeUndefined();
+    });
+
+    it('handle429 delegates to the same path', async () => {
+      const rm = new RetryManager(
+        'test-key',
+        mockPersistor,
+        defaultRateLimitConfig,
+        defaultBackoffConfig,
+        mockLogger
+      );
+
+      const spy = jest.spyOn(rm, 'handleRetryAfter');
+      await rm.handle429(15);
+      expect(spy).toHaveBeenCalledWith(15);
+    });
+  });
+
   describe('handle429', () => {
     it('increments retry count', async () => {
       const now = 1000000;

packages/core/src/plugins/SegmentDestination.ts

@@ -34,14 +34,22 @@ export const SEGMENT_DESTINATION_KEY = 'Segment.io';
 type BatchResult = {
   batch: SegmentEvent[];
   messageIds: string[];
-  status: 'success' | '429' | 'transient' | 'permanent' | 'network_error';
+  // 'retry_after' = a retryable response that carried a Retry-After header
+  // (429 or any other retryable code), so we wait the server-directed time
+  // instead of computing exponential backoff.
+  status:
+    | 'success'
+    | 'retry_after'
+    | 'transient'
+    | 'permanent'
+    | 'network_error';
   statusCode?: number;
   retryAfterSeconds?: number;
 };
 
 type ErrorAggregation = {
   successfulMessageIds: string[];
-  rateLimitResults: BatchResult[];
+  serverDirectedResults: BatchResult[];
   hasTransientError: boolean;
   permanentErrorMessageIds: string[];
   retryableMessageIds: string[];
@@ -92,21 +100,36 @@ export class SegmentDestination extends DestinationPlugin {
 
     switch (classification.errorType) {
       case 'rate_limit':
+        // 429: always a server-directed wait. Default to 60s when the header
+        // is missing/invalid, preserving prior behavior.
         return {
           batch,
           messageIds,
-          status: '429',
+          status: 'retry_after',
           statusCode: res.status,
           retryAfterSeconds: retryAfterSeconds ?? 60,
         };
       case 'transient':
+        // Any other retryable code (529, 503, 408, …): if the server sent a
+        // valid Retry-After, honor it as a server-directed wait. Otherwise use
+        // exponential backoff (unchanged behavior).
+        if (retryAfterSeconds !== undefined) {
+          return {
+            batch,
+            messageIds,
+            status: 'retry_after',
+            statusCode: res.status,
+            retryAfterSeconds,
+          };
+        }
         return {
           batch,
           messageIds,
           status: 'transient',
           statusCode: res.status,
         };
       default:
+        // Permanent: drop. Retry-After is ignored on non-retryable codes.
         return {
           batch,
           messageIds,
@@ -136,13 +159,16 @@ export class SegmentDestination extends DestinationPlugin {
         retryCount,
       });
 
-      const retryAfterSeconds =
-        res.status === 429
-          ? parseRetryAfter(
-              res.headers.get('Retry-After'),
-              this.getRateLimitConfig()?.maxRetryInterval
-            )
-          : undefined;
+      // Parse Retry-After on any error response (not just 429). The header —
+      // regardless of which retryable status code carries it — is the
+      // authoritative signal for how long to wait. classifyBatchResult decides
+      // whether to honor it (retryable codes) or ignore it (permanent codes).
+      const retryAfterSeconds = res.ok
+        ? undefined
+        : parseRetryAfter(
+            res.headers.get('Retry-After'),
+            this.getRateLimitConfig()?.maxRetryInterval
+          );
 
       return this.classifyBatchResult(
         res,
@@ -173,7 +199,7 @@ export class SegmentDestination extends DestinationPlugin {
   private aggregateErrors(results: BatchResult[]): ErrorAggregation {
     const aggregation: ErrorAggregation = {
       successfulMessageIds: [],
-      rateLimitResults: [],
+      serverDirectedResults: [],
       hasTransientError: false,
       permanentErrorMessageIds: [],
       retryableMessageIds: [],
@@ -184,8 +210,8 @@ export class SegmentDestination extends DestinationPlugin {
         case 'success':
           aggregation.successfulMessageIds.push(...result.messageIds);
           break;
-        case '429':
-          aggregation.rateLimitResults.push(result);
+        case 'retry_after':
+          aggregation.serverDirectedResults.push(result);
           aggregation.retryableMessageIds.push(...result.messageIds);
           break;
         case 'transient':
@@ -256,12 +282,14 @@ export class SegmentDestination extends DestinationPlugin {
       return false;
     }
 
-    const has429 = aggregation.rateLimitResults.length > 0;
+    const hasServerDirectedWait = aggregation.serverDirectedResults.length > 0;
     let result: RetryResult | undefined;
 
-    if (has429) {
-      for (const r of aggregation.rateLimitResults) {
-        result = await this.retryManager.handle429(r.retryAfterSeconds ?? 60);
+    if (hasServerDirectedWait) {
+      for (const r of aggregation.serverDirectedResults) {
+        result = await this.retryManager.handleRetryAfter(
+          r.retryAfterSeconds ?? 60
+        );
       }
     } else if (aggregation.hasTransientError) {
       result = await this.retryManager.handleTransientError();
@@ -316,9 +344,10 @@ export class SegmentDestination extends DestinationPlugin {
       aggregation.successfulMessageIds.length -
       aggregation.permanentErrorMessageIds.length;
     if (failedCount > 0) {
-      const has429 = aggregation.rateLimitResults.length > 0;
+      const hasServerDirectedWait =
+        aggregation.serverDirectedResults.length > 0;
       this.analytics?.logger.warn(
-        `${failedCount} events will retry (429: ${has429}, transient: ${aggregation.hasTransientError})`
+        `${failedCount} events will retry (retry-after: ${hasServerDirectedWait}, transient: ${aggregation.hasTransientError})`
       );
     }
   }