Skip to content

Commit 369f01b

Browse files
MichaelGHSegclaude
MichaelGHSeg
and
claude
committed
Cap Retry-After retries, fix maxRetries default, fix tests
- Add MAX_RETRY_AFTER_RETRIES (20) safety cap to prevent infinite retries when server keeps returning Retry-After headers (node publisher and browser batched-dispatcher) - Lower node maxRetries default from 1000 to 10 - Fix convertHeaders TS warning by removing redundant any cast - Fix retryCount metadata stamped on wrong events (batch.forEach instead of buffer.map) - Update tests: X-Retry-Count always sent (0 on first attempt), 413 now non-retryable, 300 treated as success, Retry-After cap Co-Authored-By: Claude Opus 4.6 <[email protected]>
1 parent 39495a0 commit 369f01b

File tree

6 files changed

+59
-58
lines changed

6 files changed

+59
-58
lines changed

‎packages/browser/src/plugins/segmentio/__tests__/retries.test.ts‎

Lines changed: 22 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ describe('Segment.io retries 500s and 429', () => {
5757
string,
5858
string
5959
>
60-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
60+
expect(firstHeaders['X-Retry-Count']).toBe('0')
6161

6262
const secondHeaders = fetch.mock.calls[1][1].headers as Record<
6363
string,
@@ -69,7 +69,7 @@ describe('Segment.io retries 500s and 429', () => {
6969
test('delays retry on 429', async () => {
7070
jest.useFakeTimers({ advanceTimers: true })
7171
const headers = new Headers()
72-
const resetTime = 1234
72+
const resetTime = 120
7373
headers.set('Retry-After', resetTime.toString())
7474
fetch
7575
.mockReturnValueOnce(
@@ -116,7 +116,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
116116

117117
expect(fetch).toHaveBeenCalledTimes(1)
118118
const headers = fetch.mock.calls[0][1].headers as Record<string, string>
119-
expect(headers['X-Retry-Count']).toBeUndefined()
119+
expect(headers['X-Retry-Count']).toBe('0')
120120
})
121121

122122
it('T02 Retryable 500: backoff used, header increments', async () => {
@@ -140,7 +140,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
140140
string
141141
>
142142

143-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
143+
expect(firstHeaders['X-Retry-Count']).toBe('0')
144144
expect(secondHeaders['X-Retry-Count']).toBe('1')
145145
})
146146

@@ -153,7 +153,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
153153

154154
expect(fetch).toHaveBeenCalledTimes(1)
155155
const headers = fetch.mock.calls[0][1].headers as Record<string, string>
156-
expect(headers['X-Retry-Count']).toBeUndefined()
156+
expect(headers['X-Retry-Count']).toBe('0')
157157
})
158158

159159
it('T04 Non-retryable 5xx: 505', async () => {
@@ -165,7 +165,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
165165

166166
expect(fetch).toHaveBeenCalledTimes(1)
167167
const headers = fetch.mock.calls[0][1].headers as Record<string, string>
168-
expect(headers['X-Retry-Count']).toBeUndefined()
168+
expect(headers['X-Retry-Count']).toBe('0')
169169
})
170170

171171
it('T05 Non-retryable 5xx: 511 (no auth)', async () => {
@@ -177,7 +177,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
177177

178178
expect(fetch).toHaveBeenCalledTimes(1)
179179
const headers = fetch.mock.calls[0][1].headers as Record<string, string>
180-
expect(headers['X-Retry-Count']).toBeUndefined()
180+
expect(headers['X-Retry-Count']).toBe('0')
181181
})
182182

183183
it('T06 Retry-After 429: delay, header increments', async () => {
@@ -210,7 +210,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
210210
string,
211211
string
212212
>
213-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
213+
expect(firstHeaders['X-Retry-Count']).toBe('0')
214214
})
215215

216216
it('T07 Retry-After 408: delay, header increments', async () => {
@@ -239,7 +239,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
239239
string,
240240
string
241241
>
242-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
242+
expect(firstHeaders['X-Retry-Count']).toBe('0')
243243
})
244244

245245
it('T08 Retry-After 503: delay, header increments', async () => {
@@ -268,7 +268,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
268268
string,
269269
string
270270
>
271-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
271+
expect(firstHeaders['X-Retry-Count']).toBe('0')
272272
})
273273

274274
it('T09 429 without Retry-After: backoff retry, header increments', async () => {
@@ -298,7 +298,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
298298
string,
299299
string
300300
>
301-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
301+
expect(firstHeaders['X-Retry-Count']).toBe('0')
302302
expect(secondHeaders['X-Retry-Count']).toBe('1')
303303
})
304304

@@ -321,7 +321,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
321321
string,
322322
string
323323
>
324-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
324+
expect(firstHeaders['X-Retry-Count']).toBe('0')
325325
expect(secondHeaders['X-Retry-Count']).toBe('1')
326326
})
327327

@@ -344,31 +344,20 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
344344
string,
345345
string
346346
>
347-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
347+
expect(firstHeaders['X-Retry-Count']).toBe('0')
348348
expect(secondHeaders['X-Retry-Count']).toBe('1')
349349
})
350350

351-
it('T12 Retryable 4xx: 413', async () => {
351+
it('T12 Non-retryable 4xx: 413', async () => {
352352
jest.useFakeTimers({ advanceTimers: true })
353-
fetch
354-
.mockReturnValueOnce(createError({ status: 413 }))
355-
.mockReturnValue(createSuccess({}))
353+
fetch.mockReturnValue(createError({ status: 413 }))
356354

357355
await analytics.track('event')
358356
jest.runAllTimers()
359357

360-
expect(fetch.mock.calls.length).toBeGreaterThanOrEqual(2)
361-
362-
const firstHeaders = fetch.mock.calls[0][1].headers as Record<
363-
string,
364-
string
365-
>
366-
const secondHeaders = fetch.mock.calls[1][1].headers as Record<
367-
string,
368-
string
369-
>
370-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
371-
expect(secondHeaders['X-Retry-Count']).toBe('1')
358+
expect(fetch).toHaveBeenCalledTimes(1)
359+
const headers = fetch.mock.calls[0][1].headers as Record<string, string>
360+
expect(headers['X-Retry-Count']).toBe('0')
372361
})
373362

374363
it('T13 Retryable 4xx: 460', async () => {
@@ -390,7 +379,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
390379
string,
391380
string
392381
>
393-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
382+
expect(firstHeaders['X-Retry-Count']).toBe('0')
394383
expect(secondHeaders['X-Retry-Count']).toBe('1')
395384
})
396385

@@ -403,7 +392,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
403392

404393
expect(fetch).toHaveBeenCalledTimes(1)
405394
const headers = fetch.mock.calls[0][1].headers as Record<string, string>
406-
expect(headers['X-Retry-Count']).toBeUndefined()
395+
expect(headers['X-Retry-Count']).toBe('0')
407396
})
408397

409398
it('T15 Network error (IO): retried with backoff', async () => {
@@ -425,7 +414,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
425414
string,
426415
string
427416
>
428-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
417+
expect(firstHeaders['X-Retry-Count']).toBe('0')
429418
expect(secondHeaders['X-Retry-Count']).toBe('1')
430419
})
431420

@@ -450,7 +439,7 @@ describe('Standard dispatcher retry semantics and X-Retry-Count header', () => {
450439
string
451440
>
452441

453-
expect(firstHeaders['X-Retry-Count']).toBeUndefined()
442+
expect(firstHeaders['X-Retry-Count']).toBe('0')
454443
expect(secondHeaders['X-Retry-Count']).toBe('1')
455444
})
456445
})

‎packages/browser/src/plugins/segmentio/batched-dispatcher.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { BatchingDispatchConfig, createHeaders } from './shared-dispatcher'
99
const MAX_PAYLOAD_SIZE = 500
1010
const MAX_KEEPALIVE_SIZE = 64
1111
const MAX_RETRY_AFTER_SECONDS = 300
12+
const MAX_RETRY_AFTER_RETRIES = 20
1213

1314
function kilobytes(buffer: unknown): number {
1415
const size = encodeURI(JSON.stringify(buffer)).split(/%..|./).length - 1
@@ -85,6 +86,7 @@ export default function batch(
8586
let rateLimitTimeout = 0
8687
let requestCount = 0 // Tracks actual network requests for X-Retry-Count header
8788
let isRetrying = false
89+
let retryAfterRetries = 0
8890

8991
function sendBatch(batch: object[], retryCount: number) {
9092
if (batch.length === 0) {
@@ -181,6 +183,7 @@ export default function batch(
181183
async function flush(attempt = 1): Promise<unknown> {
182184
if (!isRetrying) {
183185
requestCount = 0
186+
retryAfterRetries = 0
184187
}
185188
isRetrying = false
186189
if (buffer.length) {
@@ -219,12 +222,23 @@ export default function batch(
219222
return
220223
}
221224

225+
// Safety cap: prevent infinite retries when server keeps returning Retry-After
226+
if (isRetryableWithoutCount) {
227+
retryAfterRetries++
228+
if (retryAfterRetries > MAX_RETRY_AFTER_RETRIES) {
229+
if (buffer.length > 0) {
230+
scheduleFlush(1)
231+
}
232+
return
233+
}
234+
}
235+
222236
if (isRateLimitError) {
223237
rateLimitTimeout = error.retryTimeout
224238
}
225239

226240
buffer = [...batch, ...buffer]
227-
buffer.map((event) => {
241+
batch.forEach((event) => {
228242
if ('_metadata' in event) {
229243
const segmentEvent = event as ReturnType<SegmentFacade['json']>
230244
segmentEvent._metadata = {

‎packages/browser/src/plugins/segmentio/fetch-dispatcher.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@ export default function (config?: StandardDispatcherConfig): {
2121
): Promise<unknown> {
2222
const headers = createHeaders(config?.headers)
2323
const writeKey = (body as SegmentEvent)?.writeKey
24-
const authtoken = btoa(writeKey + ':')
25-
headers['Authorization'] = `Basic ${authtoken}`
24+
if (writeKey) {
25+
const authtoken = btoa(writeKey + ':')
26+
headers['Authorization'] = `Basic ${authtoken}`
27+
}
2628

2729
if (retryCountHeader !== undefined) {
2830
headers['X-Retry-Count'] = String(retryCountHeader)

‎packages/node/src/app/analytics-node.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics {
5151
writeKey: settings.writeKey,
5252
host: settings.host,
5353
path: settings.path,
54-
maxRetries: settings.maxRetries ?? 1000,
54+
maxRetries: settings.maxRetries ?? 10,
5555
flushAt: settings.flushAt ?? settings.maxEventsInBatch ?? 15,
5656
httpRequestTimeout: settings.httpRequestTimeout,
5757
disable: settings.disable,

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -383,14 +383,13 @@ describe('error handling', () => {
383383
expect(Date.now()).toBeGreaterThanOrEqual(start + delaySeconds * 1000 - 50)
384384
})
385385

386-
it.each([
387-
{ status: 500, statusText: 'Internal Server Error' },
388-
{ status: 300, statusText: 'Multiple Choices' },
389-
])('retries non-2xx/4xx errors: %p', async (response) => {
386+
it('retries 500 errors', async () => {
390387
// Jest kept timing out when using fake timers despite advancing time.
391388
jest.useRealTimers()
392389

393-
makeReqSpy.mockReturnValue(createError(response))
390+
makeReqSpy.mockReturnValue(
391+
createError({ status: 500, statusText: 'Internal Server Error' })
392+
)
394393

395394
const { plugin: segmentPlugin } = createTestNodePlugin({
396395
maxRetries: 2,
@@ -409,9 +408,7 @@ describe('error handling', () => {
409408
expect(updatedContext.failedDelivery()).toBeTruthy()
410409
const err = updatedContext.failedDelivery()?.reason as Error
411410
expect(err).toBeInstanceOf(Error)
412-
expect(err.message).toEqual(
413-
expect.stringContaining(response.status.toString())
414-
)
411+
expect(err.message).toEqual(expect.stringContaining('500'))
415412
})
416413

417414
it('treats 1xx (<200) statuses as success (no retry)', async () => {

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

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { TokenManager } from '../../lib/token-manager'
1111
import { b64encode } from '../../lib/base-64-encode'
1212

1313
const MAX_RETRY_AFTER_SECONDS = 300
14+
const MAX_RETRY_AFTER_RETRIES = 20
1415

1516
function sleep(timeoutInMs: number): Promise<void> {
1617
return new Promise((resolve) => setTimeout(resolve, timeoutInMs))
@@ -24,22 +25,14 @@ function convertHeaders(
2425
const lowercaseHeaders: Record<string, string> = {}
2526
if (!headers) return lowercaseHeaders
2627

27-
const candidate: any = headers
28-
29-
if (
30-
typeof candidate === 'object' &&
31-
candidate !== null &&
32-
typeof candidate.entries === 'function'
33-
) {
34-
for (const [name, value] of candidate.entries() as IterableIterator<
35-
[string, any]
36-
>) {
28+
if (typeof (headers as Record<string, unknown>).entries === 'function') {
29+
for (const [name, value] of (headers as any).entries()) {
3730
lowercaseHeaders[name.toLowerCase()] = String(value)
3831
}
3932
return lowercaseHeaders
4033
}
4134

42-
for (const [name, value] of Object.entries(candidate)) {
35+
for (const [name, value] of Object.entries(headers)) {
4336
lowercaseHeaders[name.toLowerCase()] = String(value)
4437
}
4538

@@ -316,8 +309,8 @@ export class Publisher {
316309

317310
const response = await this._httpClient.makeRequest(request)
318311

319-
if (response.status >= 100 && response.status < 300) {
320-
// Successfully sent events, so exit!
312+
if (response.status >= 100 && response.status < 400) {
313+
// exit after success or 1xx/3xx (Segment should never emit these)
321314
batch.resolveEvents()
322315
return
323316
} else if (
@@ -398,6 +391,12 @@ export class Publisher {
398391
}
399392
}
400393

394+
// Safety cap: prevent infinite retries when server keeps returning Retry-After
395+
if (totalAttempts > maxRetries + MAX_RETRY_AFTER_RETRIES) {
396+
resolveFailedBatch(batch, failureReason)
397+
return
398+
}
399+
401400
const delayMs =
402401
requestedRetryTimeout ??
403402
backoff({

0 commit comments

Comments
 (0)