Skip to content

Commit 731d4ae

Browse files
abueideclaude
andcommitted
fix: remove retryStrategy config, hardcode eager; count pruned events as drops
- Remove retryStrategy from Config type, defaultConfig, and RetryManager constructor — always use eager (shortest wait) for concurrent batch errors - Count events pruned by maxTotalBackoffDuration toward droppedEventCount - Add SDD-default config tests verifying all spec status codes against defaultHttpConfig overrides (408/410/460=retry, 501/505=drop) - Update RetryManager tests for eager-only behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b876e37 commit 731d4ae

5 files changed

Lines changed: 51 additions & 107 deletions

File tree

packages/core/src/__tests__/errors-classification.test.ts

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,17 @@
11
import { classifyError, parseRetryAfter } from '../errors';
2+
import { defaultHttpConfig } from '../constants';
23

34
describe('classifyError', () => {
4-
describe('SDD-default config (statusCodeOverrides per spec)', () => {
5+
describe('SDD-default config (defaultHttpConfig overrides)', () => {
6+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
7+
const backoff = defaultHttpConfig.backoffConfig!;
8+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
9+
const rateLimit = defaultHttpConfig.rateLimitConfig!;
510
const sddConfig = {
6-
default4xxBehavior: 'drop' as const,
7-
default5xxBehavior: 'retry' as const,
8-
statusCodeOverrides: {
9-
'408': 'retry' as const,
10-
'410': 'retry' as const,
11-
'429': 'retry' as const,
12-
'460': 'retry' as const,
13-
'501': 'drop' as const,
14-
'505': 'drop' as const,
15-
},
16-
rateLimitEnabled: true,
11+
default4xxBehavior: backoff.default4xxBehavior,
12+
default5xxBehavior: backoff.default5xxBehavior,
13+
statusCodeOverrides: backoff.statusCodeOverrides,
14+
rateLimitEnabled: rateLimit.enabled,
1715
};
1816

1917
it('retries 408 (Request Timeout)', () => {

packages/core/src/backoff/RetryManager.ts

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,9 @@ export type RetryResult = 'rate_limited' | 'backed_off' | 'limit_exceeded';
2929
* - BACKING_OFF: transient error; exponential backoff until wait expires
3030
*
3131
* Designed for concurrent batch uploads (Promise.all). Multiple batches can
32-
* fail simultaneously with different errors or partially succeed. The retry
33-
* strategy (eager/lazy) controls how concurrent wait times are consolidated.
32+
* fail simultaneously with different errors or partially succeed. When
33+
* concurrent wait times conflict, the shorter wait is used (eager strategy)
34+
* to retry sooner.
3435
*
3536
* Uses a global retry counter since batches are re-chunked from the event
3637
* queue on each flush and have no stable identities.
@@ -40,20 +41,17 @@ export class RetryManager {
4041
private rateLimitConfig?: RateLimitConfig;
4142
private backoffConfig?: BackoffConfig;
4243
private logger?: LoggerType;
43-
private retryStrategy: 'eager' | 'lazy';
4444

4545
constructor(
4646
storeId: string,
4747
persistor: Persistor | undefined,
4848
rateLimitConfig?: RateLimitConfig,
4949
backoffConfig?: BackoffConfig,
50-
logger?: LoggerType,
51-
retryStrategy: 'eager' | 'lazy' = 'lazy'
50+
logger?: LoggerType
5251
) {
5352
this.rateLimitConfig = rateLimitConfig;
5453
this.backoffConfig = backoffConfig;
5554
this.logger = logger;
56-
this.retryStrategy = retryStrategy;
5755

5856
try {
5957
this.store = createStore<RetryStateData>(
@@ -293,14 +291,11 @@ export class RetryManager {
293291
}
294292

295293
/**
296-
* Consolidate two wait-until times based on retry strategy.
297-
* - 'lazy': take the longer wait (most conservative, default)
298-
* - 'eager': take the shorter wait (retry sooner)
294+
* Consolidate two wait-until times using the eager strategy:
295+
* take the shorter wait to retry sooner.
299296
*/
300297
private applyRetryStrategy(existing: number, incoming: number): number {
301-
return this.retryStrategy === 'eager'
302-
? Math.min(existing, incoming)
303-
: Math.max(existing, incoming);
298+
return Math.min(existing, incoming);
304299
}
305300

306301
private async transitionToReady(): Promise<void> {

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

Lines changed: 32 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ describe('RetryManager', () => {
166166
expect(await rm.getRetryCount()).toBe(2);
167167
});
168168

169-
it('uses longest retry-after when multiple 429s occur', async () => {
169+
it('uses shortest retry-after when multiple 429s occur (eager)', async () => {
170170
const now = 1000000;
171171
jest.spyOn(Date, 'now').mockReturnValue(now);
172172

@@ -181,11 +181,8 @@ describe('RetryManager', () => {
181181
await rm.handle429(60);
182182
await rm.handle429(120);
183183

184-
// Should wait 120s, not 60s
184+
// Eager: should use 60s (shortest)
185185
jest.spyOn(Date, 'now').mockReturnValue(now + 61000);
186-
expect(await rm.canRetry()).toBe(false);
187-
188-
jest.spyOn(Date, 'now').mockReturnValue(now + 121000);
189186
expect(await rm.canRetry()).toBe(true);
190187
});
191188

@@ -391,20 +388,22 @@ describe('RetryManager', () => {
391388
jest.spyOn(Date, 'now').mockReturnValue(now + 400);
392389
expect(await rm.canRetry()).toBe(false);
393390

394-
// Second error before first expires: 0.5 * 2^1 = 1s
395-
jest.spyOn(Date, 'now').mockReturnValue(now + 400);
391+
// Advance past first wait → transitions to READY (retryCount preserved)
392+
jest.spyOn(Date, 'now').mockReturnValue(now + 600);
393+
expect(await rm.canRetry()).toBe(true);
394+
395+
// Second error: 0.5 * 2^1 = 1s (retryCount is still 1)
396396
await rm.handleTransientError();
397397

398-
// Should now wait for the 1s from second error
399-
jest.spyOn(Date, 'now').mockReturnValue(now + 1300);
400-
expect(await rm.canRetry()).toBe(false);
401398
jest.spyOn(Date, 'now').mockReturnValue(now + 1500);
399+
expect(await rm.canRetry()).toBe(false);
400+
jest.spyOn(Date, 'now').mockReturnValue(now + 1700);
402401
expect(await rm.canRetry()).toBe(true);
403402
});
404403

405404
it('clamps backoff to maxBackoffInterval', async () => {
406-
const now = 1000000;
407-
jest.spyOn(Date, 'now').mockReturnValue(now);
405+
let t = 1000000;
406+
jest.spyOn(Date, 'now').mockReturnValue(t);
408407

409408
const config: BackoffConfig = {
410409
...defaultBackoffConfig,
@@ -418,16 +417,21 @@ describe('RetryManager', () => {
418417
mockLogger
419418
);
420419

421-
// Retry many times to exceed maxBackoffInterval
422-
// Without moving time forward so they accumulate
423-
for (let i = 0; i < 10; i++) {
420+
// Advance retryCount high enough that unclamped backoff would exceed 5s
421+
// (0.5 * 2^4 = 8s > 5s). Advance time between errors to avoid eager min.
422+
for (let i = 0; i < 5; i++) {
424423
await rm.handleTransientError();
424+
t += 10000;
425+
jest.spyOn(Date, 'now').mockReturnValue(t);
426+
await rm.canRetry(); // transition to READY
425427
}
426428

427-
// Should be clamped to 5s
428-
jest.spyOn(Date, 'now').mockReturnValue(now + 4000);
429+
// Next error: 0.5 * 2^5 = 16s, clamped to 5s
430+
await rm.handleTransientError();
431+
432+
jest.spyOn(Date, 'now').mockReturnValue(t + 4000);
429433
expect(await rm.canRetry()).toBe(false);
430-
jest.spyOn(Date, 'now').mockReturnValue(now + 6000);
434+
jest.spyOn(Date, 'now').mockReturnValue(t + 6000);
431435
expect(await rm.canRetry()).toBe(true);
432436
});
433437

@@ -506,12 +510,11 @@ describe('RetryManager', () => {
506510
});
507511
});
508512

509-
describe('retryStrategy', () => {
510-
it('defaults to lazy (uses longest wait time)', async () => {
513+
describe('concurrent wait time consolidation', () => {
514+
it('uses shortest wait time when multiple 429s occur (eager)', async () => {
511515
const now = 1000000;
512516
jest.spyOn(Date, 'now').mockReturnValue(now);
513517

514-
// No retryStrategy passed → defaults to 'lazy'
515518
const rm = new RetryManager(
516519
'test-key',
517520
mockPersistor,
@@ -523,36 +526,12 @@ describe('RetryManager', () => {
523526
await rm.handle429(60);
524527
await rm.handle429(120);
525528

526-
// Lazy: should use 120s (longest)
527-
jest.spyOn(Date, 'now').mockReturnValue(now + 61000);
528-
expect(await rm.canRetry()).toBe(false);
529-
530-
jest.spyOn(Date, 'now').mockReturnValue(now + 121000);
531-
expect(await rm.canRetry()).toBe(true);
532-
});
533-
534-
it('eager strategy uses shortest wait time', async () => {
535-
const now = 1000000;
536-
jest.spyOn(Date, 'now').mockReturnValue(now);
537-
538-
const rm = new RetryManager(
539-
'test-key',
540-
mockPersistor,
541-
defaultRateLimitConfig,
542-
defaultBackoffConfig,
543-
mockLogger,
544-
'eager'
545-
);
546-
547-
await rm.handle429(60);
548-
await rm.handle429(120);
549-
550529
// Eager: should use 60s (shortest)
551530
jest.spyOn(Date, 'now').mockReturnValue(now + 61000);
552531
expect(await rm.canRetry()).toBe(true);
553532
});
554533

555-
it('lazy strategy uses longest wait time', async () => {
534+
it('uses shortest wait time for transient errors too', async () => {
556535
const now = 1000000;
557536
jest.spyOn(Date, 'now').mockReturnValue(now);
558537

@@ -561,32 +540,7 @@ describe('RetryManager', () => {
561540
mockPersistor,
562541
defaultRateLimitConfig,
563542
defaultBackoffConfig,
564-
mockLogger,
565-
'lazy'
566-
);
567-
568-
await rm.handle429(60);
569-
await rm.handle429(120);
570-
571-
// Lazy: should use 120s (longest)
572-
jest.spyOn(Date, 'now').mockReturnValue(now + 61000);
573-
expect(await rm.canRetry()).toBe(false);
574-
575-
jest.spyOn(Date, 'now').mockReturnValue(now + 121000);
576-
expect(await rm.canRetry()).toBe(true);
577-
});
578-
579-
it('eager strategy applies to transient errors too', async () => {
580-
const now = 1000000;
581-
jest.spyOn(Date, 'now').mockReturnValue(now);
582-
583-
const rm = new RetryManager(
584-
'test-key',
585-
mockPersistor,
586-
defaultRateLimitConfig,
587-
defaultBackoffConfig,
588-
mockLogger,
589-
'eager'
543+
mockLogger
590544
);
591545

592546
// First transient: 0.5 * 2^0 = 0.5s → wait until now + 500ms
@@ -763,7 +717,7 @@ describe('RetryManager', () => {
763717
});
764718

765719
describe('mixed 429 and transient errors', () => {
766-
it('429 wait time takes precedence over shorter transient backoff', async () => {
720+
it('state stays RATE_LIMITED but wait uses shorter time (eager)', async () => {
767721
const now = 1000000;
768722
jest.spyOn(Date, 'now').mockReturnValue(now);
769723

@@ -775,20 +729,18 @@ describe('RetryManager', () => {
775729
mockLogger
776730
);
777731

778-
// Get a 429 first
732+
// Get a 429 first → RATE_LIMITED, waitUntilTime = now + 60s
779733
await rm.handle429(60);
780734
expect(await rm.getRetryCount()).toBe(1);
781735

782-
// Then a transient error before 429 expires
736+
// Transient error at t=10s → backoff = 0.5*2^1 = 1s → waitUntil = now+11s
737+
// Eager: min(now+60s, now+11s) = now+11s, state stays RATE_LIMITED
783738
jest.spyOn(Date, 'now').mockReturnValue(now + 10000);
784739
await rm.handleTransientError();
785740
expect(await rm.getRetryCount()).toBe(2);
786741

787-
// Should use the longest wait time (429's 60s)
788-
jest.spyOn(Date, 'now').mockReturnValue(now + 50000);
789-
expect(await rm.canRetry()).toBe(false);
790-
791-
jest.spyOn(Date, 'now').mockReturnValue(now + 61000);
742+
// Eager picks shorter wait, so retryable after ~11s from original now
743+
jest.spyOn(Date, 'now').mockReturnValue(now + 12000);
792744
expect(await rm.canRetry()).toBe(true);
793745
});
794746
});

packages/core/src/constants.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ export const defaultConfig: Config = {
1010
trackAppLifecycleEvents: false,
1111
autoAddSegmentDestination: true,
1212
useSegmentEndpoints: false,
13-
retryStrategy: 'lazy',
1413
};
1514

1615
export const defaultHttpConfig: HttpConfig = {

packages/core/src/plugins/SegmentDestination.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@ export class SegmentDestination extends DestinationPlugin {
184184

185185
if (expiredMessageIds.length > 0) {
186186
await this.queuePlugin.dequeueByMessageIds(expiredMessageIds);
187+
this.droppedEventCount += expiredMessageIds.length;
187188
this.analytics?.logger.warn(
188189
`Pruned ${expiredMessageIds.length} events older than ${maxAge}s`
189190
);
@@ -368,8 +369,7 @@ export class SegmentDestination extends DestinationPlugin {
368369
config?.storePersistor,
369370
httpConfig.rateLimitConfig,
370371
httpConfig.backoffConfig,
371-
this.analytics?.logger,
372-
config?.retryStrategy ?? 'lazy'
372+
this.analytics?.logger
373373
);
374374
}
375375
}

0 commit comments

Comments
 (0)