fix: address code review feedback from silesky

- Revert shared backoff.ts defaults to original values (minTimeout: 500,
  maxTimeout: Infinity) — node publisher already passes explicit params
- Clamp maxRetryCount in both rateLimitConfig and backoffConfig (0-100)
  to prevent unbounded retries from bad CDN config
- Enforce 1s minimum on Retry-After to prevent tight request loops on
  Retry-After: 0
- Remove no-op `enabled` field from ResolvedRateLimitConfig and
  ResolvedBackoffConfig — kept in input types for CDN compat
- Tighten success range from >= 100 to >= 200 (1xx are not valid final
  responses)
- Update tests for revised 1xx and Retry-After: 0 behavior

Author: MichaelGHSeg

packages/browser/src/lib/priority-queue/backoff.ts

@@ -1,8 +1,8 @@
 type BackoffParams = {
-  /** The number of milliseconds before starting the first retry. Default is 100 */
+  /** The number of milliseconds before starting the first retry. Default is 500 */
   minTimeout?: number
 
-  /** The maximum number of milliseconds between two retries. Default is 60000 (1 minute) */
+  /** The maximum number of milliseconds between two retries. Default is Infinity */
   maxTimeout?: number
 
   /** The exponential factor to use. Default is 2. */
@@ -14,6 +14,11 @@ type BackoffParams = {
 
 export function backoff(params: BackoffParams): number {
   const random = Math.random() + 1
-  const { minTimeout = 100, factor = 2, attempt, maxTimeout = 60000 } = params
+  const {
+    minTimeout = 500,
+    factor = 2,
+    attempt,
+    maxTimeout = Infinity,
+  } = params
   return Math.min(random * minTimeout * Math.pow(factor, attempt), maxTimeout)
 }

packages/browser/src/plugins/segmentio/__tests__/shared-dispatcher.test.ts

@@ -13,14 +13,12 @@ describe('resolveHttpConfig', () => {
     const resolved = resolveHttpConfig(undefined)
 
     expect(resolved.rateLimitConfig).toEqual({
-      enabled: true,
       maxRetryCount: 10,
       maxRetryInterval: 300,
       maxRateLimitDuration: 43200,
     })
 
     expect(resolved.backoffConfig).toEqual({
-      enabled: true,
       maxRetryCount: 10,
       baseBackoffInterval: 0.5,
       maxBackoffInterval: 60,
@@ -43,9 +41,7 @@ describe('resolveHttpConfig', () => {
   it('applies all defaults when called with empty object', () => {
     const resolved = resolveHttpConfig({})
 
-    expect(resolved.rateLimitConfig.enabled).toBe(true)
     expect(resolved.rateLimitConfig.maxRetryCount).toBe(10)
-    expect(resolved.backoffConfig.enabled).toBe(true)
     expect(resolved.backoffConfig.maxRetryCount).toBe(10)
     expect(resolved.backoffConfig.baseBackoffInterval).toBe(0.5)
   })
@@ -76,13 +72,11 @@ describe('resolveHttpConfig', () => {
     const resolved = resolveHttpConfig(config)
 
     expect(resolved.rateLimitConfig).toEqual({
-      enabled: false,
       maxRetryCount: 50,
       maxRetryInterval: 120,
       maxRateLimitDuration: 3600,
     })
 
-    expect(resolved.backoffConfig.enabled).toBe(false)
     expect(resolved.backoffConfig.maxRetryCount).toBe(25)
     expect(resolved.backoffConfig.baseBackoffInterval).toBe(1)
     expect(resolved.backoffConfig.maxBackoffInterval).toBe(60)
@@ -109,10 +103,8 @@ describe('resolveHttpConfig', () => {
     expect(resolved.backoffConfig.jitterPercent).toBe(5)
 
     // Defaults for missing fields
-    expect(resolved.rateLimitConfig.enabled).toBe(true)
     expect(resolved.rateLimitConfig.maxRetryInterval).toBe(300)
     expect(resolved.rateLimitConfig.maxRateLimitDuration).toBe(43200)
-    expect(resolved.backoffConfig.enabled).toBe(true)
     expect(resolved.backoffConfig.maxRetryCount).toBe(10)
     expect(resolved.backoffConfig.baseBackoffInterval).toBe(0.5)
     expect(resolved.backoffConfig.maxBackoffInterval).toBe(60)

packages/browser/src/plugins/segmentio/shared-dispatcher.ts

@@ -138,14 +138,12 @@ export interface HttpConfig {
 // --- Resolved types (all fields required, no undefined checks needed by consumers) ---
 
 export interface ResolvedRateLimitConfig {
-  enabled: boolean
   maxRetryCount: number
   maxRetryInterval: number
   maxRateLimitDuration: number
 }
 
 export interface ResolvedBackoffConfig {
-  enabled: boolean
   maxRetryCount: number
   baseBackoffInterval: number
   maxBackoffInterval: number
@@ -288,14 +286,12 @@ export function resolveHttpConfig(
 
   return {
     rateLimitConfig: {
-      enabled: rate?.enabled ?? true,
-      maxRetryCount: rate?.maxRetryCount ?? 10,
+      maxRetryCount: clamp(rate?.maxRetryCount, 10, 0, 100),
       maxRetryInterval: clamp(rate?.maxRetryInterval, 300, 0.1, 86400),
       maxRateLimitDuration: clamp(rate?.maxRateLimitDuration, 43200, 10, 86400),
     },
     backoffConfig: {
-      enabled: backoff?.enabled ?? true,
-      maxRetryCount: backoff?.maxRetryCount ?? 10,
+      maxRetryCount: clamp(backoff?.maxRetryCount, 10, 0, 100),
       baseBackoffInterval: clamp(backoff?.baseBackoffInterval, 0.5, 0.1, 300),
       maxBackoffInterval: clamp(backoff?.maxBackoffInterval, 60, 0.1, 86400),
       maxTotalBackoffDuration: clamp(

packages/core/src/priority-queue/backoff.ts

@@ -1,8 +1,8 @@
 type BackoffParams = {
-  /** The number of milliseconds before starting the first retry. Default is 100 */
+  /** The number of milliseconds before starting the first retry. Default is 500 */
   minTimeout?: number
 
-  /** The maximum number of milliseconds between two retries. Default is 60000 (1 minute) */
+  /** The maximum number of milliseconds between two retries. Default is Infinity */
   maxTimeout?: number
 
   /** The exponential factor to use. Default is 2. */
@@ -14,6 +14,11 @@ type BackoffParams = {
 
 export function backoff(params: BackoffParams): number {
   const random = Math.random() + 1
-  const { minTimeout = 100, factor = 2, attempt, maxTimeout = 60000 } = params
+  const {
+    minTimeout = 500,
+    factor = 2,
+    attempt,
+    maxTimeout = Infinity,
+  } = params
   return Math.min(random * minTimeout * Math.pow(factor, attempt), maxTimeout)
 }

packages/node/src/plugins/segmentio/__tests__/publisher.test.ts

@@ -409,7 +409,7 @@ describe('error handling', () => {
     expect(err.message).toEqual(expect.stringContaining('500'))
   })
 
-  it('treats 1xx (<200) statuses as success (no retry)', async () => {
+  it('retries 1xx (<200) statuses (not a valid final response)', async () => {
     jest.useRealTimers()
 
     makeReqSpy.mockReturnValue(
@@ -424,10 +424,9 @@ describe('error handling', () => {
     const context = new Context(eventFactory.alias('to', 'from'))
     const updatedContext = await segmentPlugin.alias(context)
 
-    expect(makeReqSpy).toHaveBeenCalledTimes(1)
-    validateMakeReqInputs(context)
-    expect(updatedContext).toBe(context)
-    expect(updatedContext.failedDelivery()).toBeFalsy()
+    // 1xx is not a valid final response — retried then failed after maxRetries
+    expect(makeReqSpy).toHaveBeenCalledTimes(3) // 1 initial + 2 retries
+    expect(updatedContext.failedDelivery()).toBeTruthy()
   })
   it('retries fetch errors', async () => {
     // Jest kept timing out when using fake timers despite advancing time.
@@ -1074,7 +1073,7 @@ describe('retry semantics', () => {
   })
 
   it('T21 Safety cap: persistent 429 with Retry-After eventually fails', async () => {
-    jest.useRealTimers()
+    jest.useFakeTimers()
     const headers = new TestHeaders()
     headers.set('Retry-After', '0')
 
@@ -1092,7 +1091,13 @@ describe('retry semantics', () => {
     })
 
     const ctx = trackEvent()
-    const updated = await segmentPlugin.track(ctx)
+    const pending = segmentPlugin.track(ctx)
+
+    // Advance past all retry-after delays (1s minimum each, up to safety cap)
+    for (let i = 0; i < 25; i++) {
+      await jest.advanceTimersByTimeAsync(1000)
+    }
+    const updated = await pending
 
     expect(makeReqSpy.mock.calls.length).toBeGreaterThan(1)
     expect(updated.failedDelivery()).toBeTruthy()

packages/node/src/plugins/segmentio/publisher.ts

@@ -285,7 +285,9 @@ export class Publisher {
   private _setRateLimitState(headers: HTTPResponse['headers']): void {
     const retryAfterSeconds = getRetryAfterInSeconds(headers)
     if (typeof retryAfterSeconds === 'number') {
-      this._rateLimitedUntil = Date.now() + retryAfterSeconds * 1000
+      // Enforce a minimum of 1 second to prevent tight loops on Retry-After: 0
+      this._rateLimitedUntil =
+        Date.now() + Math.max(retryAfterSeconds, 1) * 1000
     } else {
       // No Retry-After header — use a default backoff of 60s
       this._rateLimitedUntil = Date.now() + 60000
@@ -391,8 +393,8 @@ export class Publisher {
 
         const response = await this._httpClient.makeRequest(request)
 
-        // Per SDD: status codes 100–399 are treated as successful delivery.
-        if (response.status >= 100 && response.status < 400) {
+        // 2xx and 3xx are treated as successful delivery.
+        if (response.status >= 200 && response.status < 400) {
           // Success — clear rate-limit state
           this._clearRateLimitState()
           batch.resolveEvents()